Closed
Bug 989751
Opened 10 years ago
Closed 10 years ago
[Australis] Web Developer and Sidebar widgets - Some items in the widgets have the wrong styling
Categories
(Firefox :: Menus, defect)
Firefox
Menus
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)
5.23 KB,
patch
|
ntim
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
All the affected items have one thing in common. They have icons in their original menuitem.
Assignee | ||
Updated•10 years ago
|
Blocks: australis-cust
Comment 2•10 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•10 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•10 years ago
|
||
I often consider regressions as severe.
Comment 5•10 years ago
|
||
(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•10 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•10 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•10 years ago
|
||
They are not actual menuitems, but they are natively styled toolbar buttons.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Keywords: regressionwindow-wanted
Updated•10 years ago
|
Whiteboard: [Australis:P?] → [Australis:P4]
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8412410 -
Flags: review?(gijskruitbosch+bugs)
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
Screenshot of issue : https://docs.google.com/document/d/1ZJxifqQYY1QILombEjlSdFDRABv3h2KvC4a0UGupQSc/edit
Assignee | ||
Comment 12•10 years ago
|
||
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•10 years ago
|
||
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 14•10 years ago
|
||
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•10 years ago
|
||
Addressed review comments :)
Attachment #8412858 -
Attachment is obsolete: true
Attachment #8413225 -
Flags: review?(gijskruitbosch+bugs)
Comment 16•10 years ago
|
||
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•10 years ago
|
||
Hopefully final :)
Attachment #8413225 -
Attachment is obsolete: true
Attachment #8413921 -
Flags: review?(gijskruitbosch+bugs)
Comment 18•10 years ago
|
||
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•10 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•10 years ago
|
||
Attachment #8413921 -
Attachment is obsolete: true
Attachment #8415207 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 21•10 years ago
|
||
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
Comment 22•10 years ago
|
||
remote: https://tbpl.mozilla.org/?tree=Try&rev=e25088ef1b8a against current fx-team tip.
Assignee | ||
Comment 23•10 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•10 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 ?
Comment 25•10 years ago
|
||
(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•10 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". :)
Comment 27•10 years ago
|
||
(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
status-firefox29:
--- → wontfix
status-firefox30:
--- → affected
status-firefox31:
--- → affected
status-firefox32:
--- → affected
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: [Australis:P4] → [Australis:P4][fixed-in-fx-team]
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 28•10 years ago
|
||
The bug that caused the regression wasn't uplifted to FFX 29.
Assignee | ||
Comment 29•10 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
Comment 30•10 years ago
|
||
(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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cc04e6210700
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4][fixed-in-fx-team] → [Australis:P4]
Target Milestone: --- → Firefox 32
Assignee | ||
Comment 32•10 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?
Updated•10 years ago
|
Updated•10 years ago
|
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•10 years ago
|
Keywords: checkin-needed
Comment 33•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/507f1da14ae6 https://hg.mozilla.org/releases/mozilla-beta/rev/5ad3f17e39d2
Comment 34•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/5ad3f17e39d2
Flags: in-testsuite+
Updated•10 years ago
|
status-b2g-v1.4:
--- → fixed
Updated•10 years ago
|
Whiteboard: [Australis:P4] → [Australis:P4], verifyme
Comment 35•10 years ago
|
||
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]
Comment 36•10 years ago
|
||
(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)
Comment 37•10 years ago
|
||
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.
Description
•