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

VERIFIED FIXED in Firefox 30

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: ntim, Assigned: ntim)

Tracking

(Blocks 1 bug, {regression})

Trunk
Firefox 32
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox29 unaffected, firefox30 verified, firefox31 verified, firefox32 verified, b2g-v1.4 fixed)

Details

(Whiteboard: [Australis:P4])

Attachments

(1 attachment, 5 obsolete attachments)

Assignee

Description

5 years ago
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.
Assignee

Comment 1

5 years ago
All the affected items have one thing in common. They have icons in their original menuitem.
Assignee

Updated

5 years ago
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?]
Assignee

Comment 3

5 years ago
(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)
Assignee

Comment 4

5 years ago
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)
Assignee

Comment 6

5 years ago
(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)
Assignee

Comment 7

5 years ago
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
Assignee

Comment 8

5 years ago
They are not actual menuitems, but they are natively styled toolbar buttons.
Assignee

Updated

5 years ago
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Assignee

Updated

5 years ago
Whiteboard: [Australis:P?] → [Australis:P4]
Assignee

Comment 9

5 years ago
Posted 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+
Assignee

Comment 12

5 years ago
Posted 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)
Assignee

Comment 13

5 years ago
Posted 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+
Assignee

Comment 15

5 years ago
Posted 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+
Assignee

Comment 17

5 years ago
Posted 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+
Assignee

Comment 19

5 years ago
(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.
Assignee

Comment 20

5 years ago
Attachment #8413921 - Attachment is obsolete: true
Attachment #8415207 - Flags: review+
Assignee

Updated

5 years ago
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
Assignee

Comment 23

5 years ago
(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 :/
Assignee

Comment 24

5 years ago
(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)
Assignee

Comment 26

5 years ago
(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]
Assignee

Comment 28

5 years ago
The bug that caused the regression wasn't uplifted to FFX 29.
Assignee

Comment 29

5 years ago
(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: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4][fixed-in-fx-team] → [Australis:P4]
Target Milestone: --- → Firefox 32
Assignee

Comment 32

5 years ago
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+
Assignee

Updated

5 years ago
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.