989751 - [Australis] Web Developer and Sidebar widgets - Some items in the widgets have the wrong styling

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Tim Nguyen :ntim]

Some items are menuitems instead of toolbarbuttons in Web Developer and Sidebar widgets. These include :
Web Dev widget :
- FireFTP

Sidebars widget :
- Social API items
- Some All in one sidebar items

Reproducible with all add-ons disabled (without the addon items showing of course) and on my other computer.

Comment 1

[Tim Nguyen :ntim]

All the affected items have one thing in common. They have icons in their original menuitem.

Comment 2

[Mike de Boer [:mikedeboer]]

Tim, from your story I don't know what to do on my local build to see the (severe) problem you're talking about... So a solid STR would be great! Screenshots are also always much appreciated.

Apart from that, could you please leave the priority flags to us?

Thanks!!

Comment 3

[Tim Nguyen :ntim]

(In reply to Mike de Boer [:mikedeboer] from comment #2)
> Tim, from your story I don't know what to do on my local build to see the
> (severe) problem you're talking about... So a solid STR would be great!
> Screenshots are also always much appreciated.
> 
> Apart from that, could you please leave the priority flags to us?
> 
> Thanks!!

The STR are obvious. Install Fireftp or all in one sidebar or any social provider. Then open the web developer or sidebars widget. And see the issue.

Comment 4

[Tim Nguyen :ntim]

I often consider regressions as severe.

Comment 5

[:Gijs (he/him)]

(In reply to Tim Nguyen [:ntim] from comment #4)
> I often consider regressions as severe.

Is it actually a regression? Can you provide a regression window? And does this reproduce on trunk as well as 29 (for the developer tools, I guess, as there's no sidebar menu)

Comment 6

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Tim Nguyen [:ntim] from comment #4)
> > I often consider regressions as severe.
> 
> Is it actually a regression? Can you provide a regression window? And does
> this reproduce on trunk as well as 29 (for the developer tools, I guess, as
> there's no sidebar menu)

It might be : https://hg.mozilla.org/mozilla-central/rev/a24bf53b82c6 which caused it.
I haven't checked on 29 yet, but I'll do it as soon as I can.

Comment 7

[Tim Nguyen :ntim]

Oh, I know why it happens, the subview button class isn't getting applied. I'll try to fix this when I'll get home.

Comment 8

[Tim Nguyen :ntim]

They are not actual menuitems, but they are natively styled toolbar buttons.

Comment 9

[Tim Nguyen :ntim]

Attachment: Patch 1

Comment 10

[:Gijs (he/him)]

Comment on attachment 8412410 [details] [diff] [review]
Patch 1

Review of attachment 8412410 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

This makes sense. I have some nits, but this would be an r+ except... I'd quite like a test for this so we don't accidentally break it in future. There's already some tests for these subviews and how separators are copied in http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_981305_separator_insertion.js .

Do you think you could write another test file that uses a similar bit of code to insert a menuitem with an existing class, and verifies that the "clone" in the subview still gets the subviewbutton class (and keeps its existing class(es))? It should basically be a question of copying the test I just mentioned, stripping all the excess code, making it add a menuitem with the right class, and then checking with the ok() and is() primitives that stuff behaves as it should. Don't forget to add the extra file to http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser.ini . If you have any questions, please ping me on IRC or needinfo me here. :-)

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +121,5 @@
>        let attrVal = menuChild.getAttribute(attr);
>        if (attrVal)
>          subviewItem.setAttribute(attr, attrVal);
>      }
> +    if(menuChild.localName == "menuitem") {

Nit: space after "if" (before opening parenthesis)

Also, please add a comment before this if block that explains we're doing this /after/ copying all the attributes so that setting the class attribute doesn't override it.

@@ +123,5 @@
>          subviewItem.setAttribute(attr, attrVal);
>      }
> +    if(menuChild.localName == "menuitem") {
> +      subviewItem.classList.add("subviewbutton");
> +      addShortcut(menuChild, doc, subviewItem);

This can just stay in place, because "shortcut" isn't one of the attributes in "attrs" above.

Comment 11

[Tim Nguyen :ntim]

Screenshot of issue : https://docs.google.com/document/d/1ZJxifqQYY1QILombEjlSdFDRABv3h2KvC4a0UGupQSc/edit

Comment 12

[Tim Nguyen :ntim]

Attachment: Patch v2 with test

Added test and addressed review comments.

Haven't tested the test yet, but I'm about too.

Comment 13

[Tim Nguyen :ntim]

Attachment: Patch v2.1 with test

I accidently used "var" instead of "let". What's the difference anyway ?

Comment 14

[:Gijs (he/him)]

Comment on attachment 8412858 [details] [diff] [review]
Patch v2.1 with test

Review of attachment 8412858 [details] [diff] [review]:
-----------------------------------------------------------------

Woo! This is almost there, awesome work! In particular because my spotty availability on Friday (I was at a friends and there was an attention-deserving small child and good friends that I see rarely) meant you didn't get too many answers for the questions you had from me (sorry!).

Next one will be r+, I'm pretty sure. :-)

::: browser/components/customizableui/test/browser_989751_subviewbutton_class.js
@@ +6,5 @@
> +
> +let tempElements = [];
> +
> +function insertClassNameToMenuChildren(parentMenu) {
> +  // Last element is null to insert at the end:

This comment seems to be copied from the other test and no longer applies, so let's get rid of it. :-)

@@ +7,5 @@
> +let tempElements = [];
> +
> +function insertClassNameToMenuChildren(parentMenu) {
> +  // Last element is null to insert at the end:
> +  let els = [parentMenu.querySelector("menuitem:first-of-type"), parentMenu.querySelector("menuitem:last-of-type"), parentMenu.querySelector("menuitem:nth-of-type(2)")];

While my terminal isn't 80 characters, this is a bit long. I also don't think you need 3 items in this case; you can just pick one, right? :-)

So instead of the loop, maybe just hardcode adding the class to the first item.

@@ +10,5 @@
> +  // Last element is null to insert at the end:
> +  let els = [parentMenu.querySelector("menuitem:first-of-type"), parentMenu.querySelector("menuitem:last-of-type"), parentMenu.querySelector("menuitem:nth-of-type(2)")];
> +  for (let i = 0; i < els.length; i++) {
> +    tempElements.push(els[i]);
> +    els[i].classList.add("acustomclassnoonewilluse");

Nit: please declare:

const kCustomClass = "acustomclassnoonewilluse";

at the top of the test (next to the tempElements declaration) and use that throughout.

@@ +36,5 @@
> +    let subview = document.getElementById(subviewId);
> +    ok(subview.firstChild, "Subview should have a kid");
> +    let subviewchildren = subview.querySelectorAll("toolbarbutton");
> +    for (let i = 0; i < subviewchildren.length; i++) {
> +      ok(subviewchildren[i].classList.contains("subviewbutton"), "All items should have the subviewbutton class.");

This is good, but if this fails for whatever reason, we won't know which item misses the class. Maybe something like:

let item = subviewchildren[i];
let itemReadable = "Item '" + item.id + "' (classes: " + item.className + ")";
ok(item.classList.contains("subviewbutton"), itemReadable + " should have the subviewbutton class");

?

Additionally, please check that the kCustomClass is still there, too!

Comment 15

[Tim Nguyen :ntim]

Attachment: Patch v2.2

Addressed review comments :)

Comment 16

[:Gijs (he/him)]

Comment on attachment 8413225 [details] [diff] [review]
Patch v2.2

Review of attachment 8413225 [details] [diff] [review]:
-----------------------------------------------------------------

Great! Just one thing missing:

::: browser/components/customizableui/test/browser_989751_subviewbutton_class.js
@@ +36,5 @@
> +    let subviewchildren = subview.querySelectorAll("toolbarbutton");
> +    for (let i = 0; i < subviewchildren.length; i++) {
> +      let item = subviewchildren[i];
> +      let itemReadable = "Item '" + item.label + "' (classes: " + item.className + ")";
> +      ok(item.classList.contains("subviewbutton"), itemReadable + " should have the subviewbutton class.");

(In reply to :Gijs Kruitbosch (PTO april 28) from comment #14)
> Additionally, please check that the kCustomClass is still there, too!

So:

ok(item.classList.contains(kCustomClass), itemReadable + " should still have its own class, too.");

Comment 17

[Tim Nguyen :ntim]

Attachment: Patch v2.3

Hopefully final :)

Comment 18

[:Gijs (he/him)]

Comment on attachment 8413921 [details] [diff] [review]
Patch v2.3

Review of attachment 8413921 [details] [diff] [review]:
-----------------------------------------------------------------

So, generally once you have r+, you don't need to request review again, although in this case, perhaps it's not so bad that you did, because I still caught something. It wouldn't really break the test - you're just testing the same element's class every iteration of the loop over subviewchildren.

Anyway, r=me with the test written as (i == 0) (and mind the space before the if condition, please). You can then just upload a new patch, set the review flag to + and set the checkin-needed keyword.

Thanks!

::: browser/components/customizableui/test/browser_989751_subviewbutton_class.js
@@ +37,5 @@
> +    for (let i = 0; i < subviewchildren.length; i++) {
> +      let item = subviewchildren[i];
> +      let itemReadable = "Item '" + item.label + "' (classes: " + item.className + ")";
> +      ok(item.classList.contains("subviewbutton"), itemReadable + " should have the subviewbutton class.");
> +      if(item = subview.firstElementChild) {

Err, you meant "==" not "=", I think. Now you're assigning to the |item| variable inside the if condition (yes, this is legal JS - although I /thought/ it would throw a strict warning...)

A simpler and more correct condition would probably be "i == 0" - because the first element child of the subview isn't necessarily a toolbarbutton, and therefore the condition might never be true if we add a non-toolbarbutton header to the subview, or if we nest the toolbar buttons inside a <vbox>, or... :-)

(the rest of your code uses querySelectorAll and :first-of-type, so this works, I think)

Comment 19

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #18)
> Comment on attachment 8413921 [details] [diff] [review]
> Patch v2.3
> 
> Review of attachment 8413921 [details] [diff] [review]:
> -----------------------------------------------------------------

> :::
> browser/components/customizableui/test/browser_989751_subviewbutton_class.js
> @@ +37,5 @@
> > +    for (let i = 0; i < subviewchildren.length; i++) {
> > +      let item = subviewchildren[i];
> > +      let itemReadable = "Item '" + item.label + "' (classes: " + item.className + ")";
> > +      ok(item.classList.contains("subviewbutton"), itemReadable + " should have the subviewbutton class.");
> > +      if(item = subview.firstElementChild) {
> 
> Err, you meant "==" not "=", I think. Now you're assigning to the |item|
> variable inside the if condition (yes, this is legal JS - although I
> /thought/ it would throw a strict warning...)
Whoops, yes.

> A simpler and more correct condition would probably be "i == 0" - because
> the first element child of the subview isn't necessarily a toolbarbutton,
> and therefore the condition might never be true if we add a
> non-toolbarbutton header to the subview, or if we nest the toolbar buttons
> inside a <vbox>, or... :-)
> 
> (the rest of your code uses querySelectorAll and :first-of-type, so this
> works, I think)
Yup didn't think of that.

Comment 20

[Tim Nguyen :ntim]

Attachment: Patch v2.4 (r=gijs)

Comment 21

[Ed Morley [:emorley]]

We're now requesting (fairly-minimal, but appropriate) Try pushes for bugs wishing to make use of sheriff checkin-neededs (regardless of the diff) - removing keyword since there isn't one pasted in the bug for the latest version of the patch. [Generic message & note I'm not CCed to the bug]

Comment 22

[:Gijs (he/him)]

remote:   https://tbpl.mozilla.org/?tree=Try&rev=e25088ef1b8a

against current fx-team tip.

Comment 23

[Tim Nguyen :ntim]

(In reply to Ed Morley [:edmorley UTC+0] from comment #21)
> We're now requesting (fairly-minimal, but appropriate) Try pushes for bugs
> wishing to make use of sheriff checkin-neededs (regardless of the diff) -
> removing keyword since there isn't one pasted in the bug for the latest
> version of the patch. [Generic message & note I'm not CCed to the bug]

I don't have access to the try server :/

Comment 24

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #22)
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=e25088ef1b8a
> 
> against current fx-team tip.

...so I have to rebase my patch ?

Comment 25

[:Gijs (he/him)]

(In reply to Tim Nguyen [:ntim] from comment #24)
> (In reply to :Gijs Kruitbosch from comment #22)
> > remote:   https://tbpl.mozilla.org/?tree=Try&rev=e25088ef1b8a
> > 
> > against current fx-team tip.
> 
> ...so I have to rebase my patch ?

No! I pushed your patch to try for you, because I knew you didn't have access. The build is still running, but I expect it'll be done later tonight and then I'll probably just push it myself. No worries! :-)

(setting needinfo so I don't forget)

Comment 26

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #25)
> (In reply to Tim Nguyen [:ntim] from comment #24)
> > (In reply to :Gijs Kruitbosch from comment #22)
> > > remote:   https://tbpl.mozilla.org/?tree=Try&rev=e25088ef1b8a
> > > 
> > > against current fx-team tip.
> > 
> > ...so I have to rebase my patch ?
> 
> No! I pushed your patch to try for you, because I knew you didn't have
> access. The build is still running, but I expect it'll be done later tonight
> and then I'll probably just push it myself. No worries! :-)
> 
> (setting needinfo so I don't forget)

Thanks !
I was just confused by "against". :)

Comment 27

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #22)
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=e25088ef1b8a
> 
> against current fx-team tip.

This is green enough for me.

remote:   https://hg.mozilla.org/integration/fx-team/rev/cc04e6210700

Comment 28

[Tim Nguyen :ntim]

The bug that caused the regression wasn't uplifted to FFX 29.

Comment 29

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #28)
> The bug that caused the regression wasn't uplifted to FFX 29.

(bug 986808)

Comment 30

[:Gijs (he/him)]

(In reply to Tim Nguyen [:ntim] from comment #29)
> (In reply to Tim Nguyen [:ntim] from comment #28)
> > The bug that caused the regression wasn't uplifted to FFX 29.
> 
> (bug 986808)

Good point, thanks for clarifying!

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cc04e6210700

Comment 32

[Tim Nguyen :ntim]

Comment on attachment 8415207 [details] [diff] [review]
Patch v2.4 (r=gijs)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 986808
User impact if declined: Inconsistent styling for some items in Developer and Sidebar widgets
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): Low, JS fix with test.
String or IDL/UUID changes made by this patch: None

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/507f1da14ae6
https://hg.mozilla.org/releases/mozilla-beta/rev/5ad3f17e39d2

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/5ad3f17e39d2

Comment 35

[Andrei Vaida [:avaida]]

This issue is verified fixed on Firefox 30 Beta 2 (Build ID: 20140505140302), using:
  * Windows 7 64-bit [1],
  * Windows 8.1 64-bit [2],
  * Ubuntu 13.10 64-bit [3],
  * Mac OS X 10.9 [4].

There are specific items such as Developer > Browser Console, which despite being opened, do *not* appear as enabled in the menu panel. Other examples: App Manager, Network (only Toggle Tools appears enabled instead), Scratchpad. Is this behavior intended?


[1] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
[2] Mozilla/5.0 (Windows NT 6.3; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
[3] Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0
[4] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0

Comment 36

[:Gijs (he/him)]

(In reply to Andrei Vaida, QA [:avaida] from comment #35)
> There are specific items such as Developer > Browser Console, which despite
> being opened, do *not* appear as enabled in the menu panel. Other examples:
> App Manager, Network (only Toggle Tools appears enabled instead),
> Scratchpad. Is this behavior intended?

Yes, that's not what this bug is about, and the main menus behave the same.

Comment 37

[Andrei Vaida [:avaida]]

I was able to also confirm the fix for this bug on Nightly 32.0a1 (2014-05-27) and Aurora 31.0a2 (2014-05-27).