998687 - Edit bookmark open state uses wrong styling on Win 7

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[u428464]

Attachment: Opened state styling on Win 7

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.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

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! :)

Comment 3

[Tim Nguyen :ntim]

(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.

Comment 4

[Tim Nguyen :ntim]

Feel free to steal this in case I don't create a patch in time.

Comment 5

[Tim Nguyen :ntim]

Attachment: Patch

I had a much busier schedule than I expected. Sorry for the delay :/

Comment 6

[Tim Nguyen :ntim]

Hey Jared, it seems that you are busy :) Should I defer this review to someone else ?

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 8

[Tim Nguyen :ntim]

(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 9

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 10

[Carsten Book [:Tomcat]]

hey guys, do we need a try link/run for this change ?

Comment 11

[Mike de Boer [:mikedeboer]]

(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!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Can we please? :P

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Sorry, I'm clearly not nearly well-caffeinated enough yet today :)

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

Comment 14

[Mike de Boer [:mikedeboer]]

https://tbpl.mozilla.org/?tree=Try&rev=0fa6213e0d59

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Tim Nguyen :ntim]

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

Comment 17

[henryfhchan]

[disabled=true] when all the other rules are just simply [disabled]?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a9b03fc36271
https://hg.mozilla.org/releases/mozilla-beta/rev/ebf5c0367290

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/ebf5c0367290

Comment 20

[Andrei Vaida [:avaida]]

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

Comment 21

[henryfhchan]

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.

Comment 22

[henryfhchan]

Attachment: 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.

Comment 23

[Tim Nguyen :ntim]

(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.