Parser related issues (part 1)

[Drugoy]

First of all, let's clarify how you want me to report parser related issues?

1. I open separate bugs for each issue (if I find 5 issues - I file 5 bugs).
2. I open separate bugs for all the found issues for one moment (I found 3 issues at once - I'll report all of them in a single bug, some time passes and I found 5 more issues - I file them in another bug).
3. We can have a single bug for all parser related issues and keep it always open, even if all the issues are fixed.

As for the found bugs: B̶u̶g̶ ̶1̶ ' symbol inside of the commentary breaks the code. It may cause different errors like:

1. Comment start without an ending.
2. Unmatched {
3. Unmatched '

Example code:

@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
@-moz-document url("about:addons"),url("chrome://mozapps/content/extensions/extensions.xul") {
/* ' */
}

This code will drop errors №1 and №2, delete 2nd and 4th lines - there will show up error №3.

B̶u̶g̶ ̶2 Create new style > paste some code with errors > hit "preview" button Actual result: get offered to save the style. Expected result: just preview the style and show errors, if there are some.

B̶u̶g̶ ̶3 I can't save a style with xhtml namespace: when I hit "save" nothing happens. When I hit "exit" I get a prompt "Do you want to save the style before closing?" with 3 buttons and "save" is one of them, but hitting it does the same as "cancel":

@namespace url(http://www.w3.org/1999/xhtml);
hello { background-color: yellow !important; }

I see this error in Error console: Error: match is null Source File: chrome://userstylemanager/content/editor.js Line: 1532

B̶u̶g̶ ̶4̶ I can't save a style that start not with a namespace.

#banner { display: none !important; }

The above style is quite legal: it should affect both: interface and web pages. But if you try to save such a style - your extension opens a prompt user to specify namespace and the prompt contains only 2 buttons: one for XUL and the other for XHTML. This is pretty useful for newbies, but it's quite bad that there is no a 3rd (maybe small) button like "save it just as it is".

B̶u̶g̶ ̶5 If you create a new style, make textarea blank (by default there is a commentary in the top line), and write

test{

Then the smart editor won't add a closing curve bracket "}". But if you do the same actions but not on the first line - it will work fine. As mentioned in the above bug - the user may want to write a style without namespace, so it's bad that some functions do not work on the line where the parser currently expects to see the namespace.

B̶u̶g̶ ̶6̶ Style names can't contain some symbols, like ":", but there is no error/notification about that: instead the style acts like it was saved and gets listed in the "Manage styles" window, but if you'll open - you'll see that it is blank, e.g. it didn't get saved. You should either add a notification asking to change style's name or (what I like more) just remove any limits for styles' names and allow them to contain any symbols.

B̶u̶g̶ ̶7̶ Renaming a style doesn't rename it in the "Manage styles" window: I had a style with the name

App Button: small with a dropmarker icon + transparent in inactive window

but I got stuck with the bug 6, so I changed it's name to

App Button small with a dropmarker icon and transparent in inactive window

so now in the "Manage styles" window I see the 1st name and when I click "edit" - I see the 2nd name.

Bug 8. I'm not sure if it's parser related issue, but… Hitting "preview" counts as an action, so when you hit it - the next time you hit CTRL+Z it will undo that action (e.g. undo visually nothing).

Bug 9. I tried to rename a style in the "Manage styles" window. When I was renaming it - I used "delete" key to delete the symbol next to caret ("backspace" deletes the symbol BEFORE the caret), and I suddenly got prompted whether I'm sure that I want to delete the style. Delete button shouldn't initiate style deletion, if you are in the rename mode.

[Drugoy]

I found all the above bugs when I tried to migrate from Stylish to USM. Yet, I can't migrate, because some styles can't be installed into USM due to the above bugs.

[grssam]

1. Will fix.
2. This is the expected behavior, and happens in all the editors. You cannot udo a preview/save call. What do you expect to undo save when hitting undo after save call ?
3. Okay, this one is only strings issue, it does not save the file. Its just that the string used in the confirm dialog box is same. I will fix it.
4. Will fix.
5. Is that valid ? If yes, then I think I should add a button.
6. Hmm, regexp issue. Will fix.
7. Yes I forgot to replace ":" with "". Will do.
8. Yes, inside the editor, the name corresponds to the actual name of the file on disk, while on the options window, the name corresponds to the Displayed name of the style.

[Drugoy]

2). You got me wrong: I also don't think that a user should be able to undo preview/save action, that's why I wrote this bug report, because currently if you type some text, hit preview 5 times and then hit undo - nothing will change. You will have to hit undo 5 more times in order to see any changes in the code.

5). Yes, it's valid, that's why I reported that. When you add namespace you actually limit your style's area of effect. But that is okay, if you want not to limit it.

8). Uhm, I get it, but it's a bit confusing. Could you then somehow point that in the Editor's window you specify not the style's name but the style file's name? Or I'd suggest you to escape any confusion at all by just making the style always have the same name as it's file has.

1)., 3)., 4)., 6)., 7). - thanks, will wait for the fixes to land.

p.s.: I saw you've uploaded 0.8 to AMO and it still to be reviewed - is there some kind of query for reviewing? Do you lose your query if you re-upload a newer version with some more changes?

[grssam]

2. Ah, I tried only pressing Preview one time so did not see the issue.
3. Okay.
4. I think I should allow two different names but make it clear somehow differently about the same.

Sadly, yes. I ts a bug in amo site. It did not use to happen a few months ago.

9. Hmm yes.

[grssam]

I have fixed all issues except the one where when you press delete while in name editing mode and when you have to undo x-1 times for every x continues preview clicks. I will fix rest after implementing color picker.

[Drugoy]

Bug 1. Fixed completely.

Bug 2. It's fixed only partially, here is an example code that still prompts to save the style:

@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
@-moz-document url("about:addons"),url("chrome://mozapps/content/extensions/extensions.xul") {
/* '''j"'"" */
color: red !important;}
}

Bug 3. It's fixed only partially, it works fine now if you do not write any namespace at all, but this style:

@namespace url(http://www.w3.org/1999/xhtml);
hello { background-color: yellow !important; }

gets transformed to:

url(http://www.w3.org/1999/xhtml);
@-moz-document domain('') {
hello { background-color: yellow !important; }
}

when I click "preview". What if a user doesn't want to specify a domain? or if he wants to specify a prefix or an exact url? Please, do not forget that there are a few more namespace related rules (or even a combination of the multiple rules as a single one):

@-moz-document url(http://this.is/an/exact/url/)
@-moz-document url-prefix(http://thisisaprefixrule)
@-moz-document url(http://this.is/an/exact/url/), url-prefix(http://thisisaprefixrule)

Maybe it's worth adding some kind of a micro-wizard like stylish has? Where user could define the rules and the wizard will transform them into the code.

Bug 4. Fixed, but it's a bit annoying to always get a prompt when you just click "preview" button, and users usually hit it quite often, while they write a single style. Could you make this prompt appear only if the user hits "save" button, not "preview"?

Bug 5. Fixed completely.

Bug 6. Not fixed. Try to save a style with the name 123:456;789 - it pretends like it's saved, but when you close the editor's window and try to edit this style - it opens absolutely blank, so the code gets lost.

Bug 7. Not fixed. I remember we discussed it somewhere, that the style's name should match the file's name, so the name of the style in the "manage" window should be exactly the same as in the "editor" window. Always.

Bug 10. I tried to delete 2 styles at once: I selected 2 line in the "manager" window and hit "delete" button.

Expected result: Styles get deleted after I click "confirm" in the "are you sure?" prompt.

Actual result: Style were not deleted + I got this error in the Error Console:

Error: styleSheetList[index] is undefined
Source File: chrome://userstylemanager/content/options.js
Line: 379

p.s.: since you have enough rights to edit my messages - a small ask: please, cross out the fixed issues in my post for easier navigation among them.

[grssam]

Bug 2. I copy pasted the code you provided, clicked Preview and the message clearly mentioned Click OK to save/preview. Also even though you are not able to see preview, be assured that clicking preview will only result in a preview. It will not save the style unless you click Save.

Bug 3. Yes you are right, I will fix it. Regarding allowing different types of prefixes/domains etc. They all are supported, there is something else going on here.

Bug 4. Will think of something better.

Bug 6. Yes still there, I wonder why though. Will dig deeper.

Bug 7. User should have control over the name as for the matter of fact, in the file name, you cannot have a ':' and many other characters.

Bug 10. Cannot reproduce this one. I tried all combinations, 2 items selected, 3 items selected, far apart, consecutive and each time it happened the way it should be, a confirm prompt and then files got deleted.

[Drugoy]

Bug 2. Oh, I messed up this bug with Bug 4, because they are pretty connected. I agree, that the prompt constructed absolutely correctly at the moment. My point is that there should be no prompt AT ALL when the user hits "preview". +1 to my point is that if the code has no errors and the user hits "preview" button - the prompt won't appear at all. So why should it appear if the code has some errors? Just notify the user about them at the bottom of the window and that's it. I've just also noticed this propmt:

There are some errors in the CSS.
They have been highlighted.
Press OK to save/preview the style or Cancel to correct the errors and save later.

and I don't like it either.

Bug 3. + Actually, current variant with prompt appearing upon clicking "save" is a bit annoying. Could you make it a bit easier for a user to use some kind of a wizard, that understands easy rules and converts them into the valid CSS code?

Bug 7. There are 2 subjects.

1. I get your point about the limitations for the file name and I agree with it. I suggest just ignoring all those symbols that can't be used in the file name: e.g. they should be removed from the file name, but should be visible in the manager window and editor's window.
2. I don't like the current realization: it allows you to physically write in the prohibited symbols. I suggest to copy Windows' explorer behavior: when the user attempts to write in a prohibited symbol, or paste it from the clipboard (separately or as a part of some phrase) - it shows a small popup, that says "you can't enter the following symbols: \ / " < : * > | ?" and it blocks the input for those prohibited symbols. And what USM does is that it allows you to write it in, so in some cases (can't yet define exact examples, but I just experienced that a few times) when you click "save" - you get your code lost.

Bug 10. Well, it's 100% reproducible for me. Will try to figure out more about this.

Bug 11. Microbug: you've set hotkey "double click" to enter the "edit name" mode (when in "manager" window). This makes double right click or double middle click do the same. I think you should limit it to "double LEFT click", not "double ANY click".

[grssam]

Bug 2. It is not a bug then. I will streamline the editor once all bugs are solved and other important issues/features are added. Currently I am upto integrating userstyles.org.

Bug 7. I actually remove those unallowed words . Only some (like : ) are not being removed.

Bug 10. Please put a screenshot of the selection.

Bug 11. I am not handling the double click. It is a default behavior of a Tree type view and I cannot detect the button pressed. May be I will streamline this also till version 1.0

[Drugoy]

Do you keep track of the unfixed issues I've listed here or should I file them separately?

[grssam]

I have them under control , but they will take some time as I have my exams approaching.

[grssam]

This solves bug 7 also. Can you reproduce bug 10 everytime? I will need more information on it as I cannot reproduce it.

[Drugoy]

I can confirm bugs 3 and 6 are completely fixed now. Bug 7 is fixed, however, I'd suggest you to block the prohibited symbols from being input into the text field at all. So when you type a prohibited symbol (any of these: \ / " < : * > | ?) - it won't even get typed in into that field and the user instead should see a popup that explains why this symbol can't be used. Bug 10 is still reproducible for me. Not 100%, but 99% now: once I've managed to delete 2 styles at once, but I couldn't reproduce that.

[grssam]

But while creating a totally new user style, the user might want to type the name containing ":" which will appear in the name displayed in the Manage window.

[Drugoy]

Oh, I didn't know that the prohibited symbols typed in into file name field remain for style name field and get properly ignored in the file name. Then it's awesome as it is now.

[Drugoy]

I still find it quite annoying that every time I create a new style to quickly test something and I hit "preview" - I get prompted about namespace selection. Since you have pretty much unused space to the right from "File Edit Tools" buttons and to the left from findbar - could you use this space to add a notification there like "no namespace specified", which could be clickable and would invoke that prompt where the user could choose a namespace. That would be even better, since it will be:

1. not annoying
2. if you plan to add a namespace configurator later - this button could be changed to open a dropdown list with commands, instead of a popup.

[grssam]

But doesn't that namespace asking prompt come only once per stylesheet ? But well your idea is also good.

[Drugoy]

Yes it does. And it's still annoying. Plus, If you once clicked to "preview as it is" - you can't later select a namespace.

[grssam]

Yes you can change later. By either typing it manually, or by using the Tools menu.