Closed Bug 998687 Opened 11 years ago Closed 11 years ago

Edit bookmark open state uses wrong styling on Win 7

Categories

(Firefox :: Theme, defect)

All
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox30 --- verified
firefox31 --- verified
firefox32 --- verified

People

(Reporter: u428464, Assigned: ntim)

References

Details

(Whiteboard: [Australis:P3-])

Attachments

(3 files)

Attached image Opened state styling on Win 7 (deleted) —
Since the open state was introduced by bug 985509, the styling used on Win 7 is not the right one. It seems that the Win 8 one is used instead.
Whiteboard: [Australis:P:?]
Blocks: 985509
Status: UNCONFIRMED → NEW
Ever confirmed: true
This is most likely happening because the same rules need to be duplicated for Windows 7, and until then the Windows 8 ones will be the only ones applying and thus aren't being overridden.
Whiteboard: [Australis:P:?] → [Australis:P3-]
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Hey Tim, just to let you know that we'll need a fix for this bug in order to uplift bug 985509 to Aurora. If you could please put this bug at the top of your list, it would be greatly appreciated! :)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2) > Hey Tim, just to let you know that we'll need a fix for this bug in order to > uplift bug 985509 to Aurora. If you could please put this bug at the top of > your list, it would be greatly appreciated! :) Well I was thinking of addressing this in bug 949151 so I'll have less re-basing work to do.
Feel free to steal this in case I don't create a patch in time.
Attached patch Patch (deleted) — Splinter Review
I had a much busier schedule than I expected. Sorry for the delay :/
Attachment #8420308 - Flags: review?(jaws)
Hey Jared, it seems that you are busy :) Should I defer this review to someone else ?
Flags: needinfo?(jaws)
Comment on attachment 8420308 [details] [diff] [review] Patch Review of attachment 8420308 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay on review here, I was away from my Win7 machine for quite some time. ::: browser/themes/windows/browser.css @@ +768,5 @@ > border-top-left-radius: 0; > border-bottom-left-radius: 0; > } > > + #nav-bar .toolbarbutton-1:not([disabled=true]) > .toolbarbutton-menubutton-button[open] + .toolbarbutton-menubutton-dropmarker > .dropmarker-icon, As this is a less-than-Windows8 issue, why does this line need to be added since it will also apply to Windows8 here?
Flags: needinfo?(jaws)
Attachment #8420308 - Flags: review?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7) > Comment on attachment 8420308 [details] [diff] [review] > Patch > > Review of attachment 8420308 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry for the delay on review here, I was away from my Win7 machine for > quite some time. > > ::: browser/themes/windows/browser.css > @@ +768,5 @@ > > border-top-left-radius: 0; > > border-bottom-left-radius: 0; > > } > > > > + #nav-bar .toolbarbutton-1:not([disabled=true]) > .toolbarbutton-menubutton-button[open] + .toolbarbutton-menubutton-dropmarker > .dropmarker-icon, > > As this is a less-than-Windows8 issue, why does this line need to be added > since it will also apply to Windows8 here? In case you didn't notice, there's 2 space indenting everywhere. Which means it's included in the media query. I think it's the patch formatting that confuses you.
Comment on attachment 8420308 [details] [diff] [review] Patch Review of attachment 8420308 [details] [diff] [review]: ----------------------------------------------------------------- I had actually applied the patch to get more context but ended up looking at the wrong ruleset. Thanks for reaffirming. I tested it and it looks good.
Attachment #8420308 - Flags: review?(jaws) → review+
Keywords: checkin-needed
hey guys, do we need a try link/run for this change ?
(In reply to Carsten Book [:Tomcat] from comment #10) > hey guys, do we need a try link/run for this change ? No. Thanks for asking!
Can we please? :P
Keywords: checkin-needed
Sorry, I'm clearly not nearly well-caffeinated enough yet today :) https://hg.mozilla.org/integration/fx-team/rev/ea8e8684eea3
Whiteboard: [Australis:P3-] → [Australis:P3-][fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3-][fixed-in-fx-team] → [Australis:P3-]
Target Milestone: --- → Firefox 32
Comment on attachment 8420308 [details] [diff] [review] Patch [Approval Request Comment] Bug caused by (feature/regressing bug #): Needed for bug 985509. User impact if declined: Wrong open state on Win 7 Testing completed (on m-c, etc.): On m-c Risk to taking this patch (and alternatives if risky): Low, css only String or IDL/UUID changes made by this patch: None
Attachment #8420308 - Flags: approval-mozilla-beta?
Attachment #8420308 - Flags: approval-mozilla-aurora?
[disabled=true] when all the other rules are just simply [disabled]?
Attachment #8420308 - Flags: approval-mozilla-beta?
Attachment #8420308 - Flags: approval-mozilla-beta+
Attachment #8420308 - Flags: approval-mozilla-aurora?
Attachment #8420308 - Flags: approval-mozilla-aurora+
Keywords: checkin-needed
This issue is verified fixed on Windows 7 64-bit, using: * Firefox 30 Beta 6 build2 (Build ID: 20140520115057) [1], * Aurora 31.0a2 (2014-05-20) [2], * Nightly 32.0a1 (2014-05-20) [3]. One note though, the selected state of the star button from Aurora looks slightly different than the one from beta or nightly, screenshot here [4]. Any thoughts on this? [1] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 [2] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 [3] Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:32.0) Gecko/20100101 Firefox/32.0 [4] http://goo.gl/FOoK4B
Status: RESOLVED → VERIFIED
Keywords: verifyme
There's still one glaring problem: the right hand border of the depressed button is too light. Compare this with the mousedown state of the bookmark button and the opened state of the bookmarks menu button.
Attached image bug-998687.png (deleted) —
[1] bookmark button, mousedown [2] bookmark button, opened [3] bookmarks menu button, mousedown/opened [4] bookmark button, hover Problems: [1] has an unnecessary border-radius on topright and bottomright (compared to [4] and [3]) [2] has a missing border-right, causing the separator to show through.
(In reply to henryfhchan from comment #22) > Created attachment 8426145 [details] > bug-998687.png > > [1] bookmark button, mousedown > [2] bookmark button, opened > [3] bookmarks menu button, mousedown/opened > [4] bookmark button, hover > > Problems: > [1] has an unnecessary border-radius on topright and bottomright (compared > to [4] and [3]) > [2] has a missing border-right, causing the separator to show through. Please file a new bug. We have the same bug on Windows 8.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: