Closed Bug 989751 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Menus, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox29 --- unaffected
firefox30 --- verified
firefox31 --- verified
firefox32 --- verified
b2g-v1.4 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [Australis:P4])

Attachments

(1 file, 5 obsolete files)

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.
All the affected items have one thing in common. They have icons in their original menuitem.
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!!
Flags: needinfo?(ntim007)
Whiteboard: [Australis:P1] → [Australis:P?]
(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.
Flags: needinfo?(ntim007)
I often consider regressions as severe.
(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)
Flags: needinfo?(ntim007)
(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.
Flags: needinfo?(ntim007)
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.
Summary: [Australis] Web Developer and Sidebar widgets - Some items in the widgets are menuitems instead of toolbarbuttons → [Australis] Web Developer and Sidebar widgets - Some items in the widgets have the wrong styling
They are not actual menuitems, but they are natively styled toolbar buttons.
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Whiteboard: [Australis:P?] → [Australis:P4]
Attached patch Patch 1 (obsolete) — Splinter Review
Attachment #8412410 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8412410 - Flags: review?(gijskruitbosch+bugs) → feedback+
Attached patch Patch v2 with test (obsolete) — Splinter Review
Added test and addressed review comments.

Haven't tested the test yet, but I'm about too.
Attachment #8412410 - Attachment is obsolete: true
Attachment #8412854 - Flags: review?(gijskruitbosch+bugs)
Attached patch Patch v2.1 with test (obsolete) — Splinter Review
I accidently used "var" instead of "let". What's the difference anyway ?
Attachment #8412854 - Attachment is obsolete: true
Attachment #8412854 - Flags: review?(gijskruitbosch+bugs)
Attachment #8412858 - Flags: review?(gijskruitbosch+bugs)
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!
Attachment #8412858 - Flags: review?(gijskruitbosch+bugs) → feedback+
Attached patch Patch v2.2 (obsolete) — Splinter Review
Addressed review comments :)
Attachment #8412858 - Attachment is obsolete: true
Attachment #8413225 - Flags: review?(gijskruitbosch+bugs)
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.");
Attachment #8413225 - Flags: review?(gijskruitbosch+bugs) → review+
Attached patch Patch v2.3 (obsolete) — Splinter Review
Hopefully final :)
Attachment #8413225 - Attachment is obsolete: true
Attachment #8413921 - Flags: review?(gijskruitbosch+bugs)
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)
Attachment #8413921 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
Attachment #8413921 - Attachment is obsolete: true
Attachment #8415207 - Flags: review+
Keywords: checkin-needed
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]
Keywords: checkin-needed
remote:   https://tbpl.mozilla.org/?tree=Try&rev=e25088ef1b8a

against current fx-team tip.
(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 :/
(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 ?
(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)
Flags: needinfo?(gijskruitbosch+bugs)
(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". :)
(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
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: [Australis:P4] → [Australis:P4][fixed-in-fx-team]
The bug that caused the regression wasn't uplifted to FFX 29.
(In reply to Tim Nguyen [:ntim] from comment #28)
> The bug that caused the regression wasn't uplifted to FFX 29.

(bug 986808)
Depends on: 986808
(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!
https://hg.mozilla.org/mozilla-central/rev/cc04e6210700
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4][fixed-in-fx-team] → [Australis:P4]
Target Milestone: --- → Firefox 32
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
Attachment #8415207 - Flags: approval-mozilla-beta?
Attachment #8415207 - Flags: approval-mozilla-aurora?
Attachment #8415207 - Flags: approval-mozilla-beta?
Attachment #8415207 - Flags: approval-mozilla-beta+
Attachment #8415207 - Flags: approval-mozilla-aurora?
Attachment #8415207 - Flags: approval-mozilla-aurora+
Keywords: checkin-needed
Whiteboard: [Australis:P4] → [Australis:P4], verifyme
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
Flags: needinfo?(ntim007)
QA Contact: andrei.vaida
Whiteboard: [Australis:P4], verifyme → [Australis:P4]
(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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(ntim007)
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).
You need to log in before you can comment on or make changes to this bug.