Update toolbar button hover/pressed states on Windows 10

VERIFIED FIXED in Firefox 43

Status

()

P3
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Firefox 43
Unspecified
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox40+ wontfix, firefox41+ wontfix, firefox42 wontfix, firefox43 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Updated

3 years ago
Blocks: 1173744

Updated

3 years ago
Priority: -- → P1
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Depends on: 1178004
needinfo for the hsla/rgba colors as we talked about on IRC
Flags: needinfo?(shorlander)
Priority: P1 → P2
Flags: needinfo?(shorlander)
Flags: qe-verify+
QA Contact: cornel.ionce
Assignee: ntim.bugs → nobody
Status: ASSIGNED → NEW
Sharing some of the research I've done :

:root {
  --toolbarbutton-hover-background: rgba(0,0,0,0.1);
  --toolbarbutton-hover-bordercolor: transparent;
  --toolbarbutton-hover-boxshadow: 0 0 0 2px rgba(0,0,0,0.2);
  --toolbarbutton-active-background: rgba(0,0,0,0.15);
  --toolbarbutton-active-bordercolor: transparent;
  --toolbarbutton-active-boxshadow: 0 0 0 2px rgba(0,0,0,0.3)
}

This works, but there are some issues :
- Forward button looks broken with this style
- The toolbar buttons have more height than the mockup
- The split buttons (bookmarks button) don't look right
Just a quick note since I've just looked at this: the buttons have a 1px border radius in the mockup, so let's include that as well.
No longer blocks: 1077146
This could work for the bookmarks button :
<div>
<button>F</button>
<button>G</button>
</div>

<style>
button {
  background: rgba(0,0,0,0.1);
  border: none;
  float: left;
  padding: 6px 10px;
}
button:first-child {
  box-shadow: -1.5px 0 0 2px rgba(0,0,0,0.2);

}
button:last-child {
  box-shadow: 1.5px 0 0 2px rgba(0,0,0,0.2);
}
</style>

If this doesn't, we'll have to use an actual 2px border.
Tracking 40 as Philipp flagged this as a bug that we need for Windows 10 support in this release.
status-firefox40: --- → affected
status-firefox42: --- → affected
tracking-firefox40: --- → +
Unfortunately, this is too late for 40. We have time to take a fix for 41 if it is available soon.
status-firefox40: affected → wontfix
tracking-firefox41: --- → +
-> P3, and a candidate for being in a UI polish clusterbug.
Priority: P2 → P3

Comment 8

3 years ago
Dolske, Dao: I am following up on FF41 tracked bugs. Since this is win10 related and was not fixed in FF40, it would be nice if we fixed this for FF41. What do you think?
Flags: needinfo?(dolske)
Flags: needinfo?(dao)
Yes, that's why it's on our list of things we'd like to fix in 41 (bug 1192839).
Flags: needinfo?(dolske)
Flags: needinfo?(dao)
(Assignee)

Comment 10

3 years ago
Created attachment 8657613 [details] [diff] [review]
patch
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #8657613 - Flags: review?(jaws)
Also, I'm not quite sure if we want this on Windows 8.
(Assignee)

Comment 13

3 years ago
I don't want Windows 8 to be the odd one out. We should let it follow the style for XP/Vista/7 or that for 10, and I think 10 probably makes more sense.
Comment on attachment 8657613 [details] [diff] [review]
patch

Review of attachment 8657613 [details] [diff] [review]:
-----------------------------------------------------------------

This adds an awkward splitter on menu-buttons, http://screencast.com/t/OZyi6z95Jt
Attachment #8657613 - Flags: review?(jaws) → review-
(Assignee)

Comment 15

3 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> Comment on attachment 8657613 [details] [diff] [review]
> patch
> 
> Review of attachment 8657613 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This adds an awkward splitter on menu-buttons,
> http://screencast.com/t/OZyi6z95Jt

It's just two adjacent borders, doesn't look that awkward to me. If this is the only issue, we should get this landed and fine-tune in a separate bug.
(Assignee)

Updated

3 years ago
Attachment #8657613 - Flags: review- → review?(jaws)
I disagree. This is tracking firefox41 and we would want to uplift this but not with that regression which will appear in the default set.
(Assignee)

Comment 17

3 years ago
Not targeting 41 then.
No longer blocks: 1192839
(Assignee)

Updated

3 years ago
status-firefox41: affected → wontfix
(Assignee)

Comment 18

3 years ago
Stephen, Philipp, do we still want the 2px thick border on hover? We had this for the location and search bars but then reverted it to 1px.
Flags: needinfo?(shorlander)
Flags: needinfo?(philipp)
(In reply to Dão Gottwald [:dao] from comment #18)
> Stephen, Philipp, do we still want the 2px thick border on hover? We had
> this for the location and search bars but then reverted it to 1px.

I think we should use the 2px border on buttons only on the pressed state. We can use the same transition of the inner shadow as we do on the URL bar.
(Unless Stephen has a strong opinion in doing something different)
Flags: needinfo?(philipp)
(Assignee)

Comment 20

3 years ago
Created attachment 8659744 [details] [diff] [review]
patch v2
Attachment #8657613 - Attachment is obsolete: true
Attachment #8659744 - Flags: review?(jaws)
(Assignee)

Comment 21

3 years ago
Comment on attachment 8659744 [details] [diff] [review]
patch v2

changing reviewer since Jared is "mostly away" for a couple of days
Attachment #8659744 - Flags: review?(jaws) → review?(gijskruitbosch+bugs)

Comment 22

3 years ago
Comment on attachment 8659744 [details] [diff] [review]
patch v2

Review of attachment 8659744 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but see the question below.

::: browser/themes/windows/browser.css
@@ +50,5 @@
>  
>    --urlbar-separator-color: hsla(0,0%,16%,.2);
>  }
>  
> +#nav-bar[brighttext] {

Shouldn't this happen for the other toolbars as well?
Attachment #8659744 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 23

3 years ago
(In reply to :Gijs Kruitbosch from comment #22)
> Comment on attachment 8659744 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8659744 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me but see the question below.
> 
> ::: browser/themes/windows/browser.css
> @@ +50,5 @@
> >  
> >    --urlbar-separator-color: hsla(0,0%,16%,.2);
> >  }
> >  
> > +#nav-bar[brighttext] {
> 
> Shouldn't this happen for the other toolbars as well?

We don't use that custom toolbar button style on other toolbars, except for the find bar but I don't think we have the brighttext attribute there...
Flags: needinfo?(shorlander)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fd3764cfdcc5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Comment on attachment 8659744 [details] [diff] [review]
patch v2

Review of attachment 8659744 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/windows/browser.css
@@ +50,5 @@
>  
>    --urlbar-separator-color: hsla(0,0%,16%,.2);
>  }
>  
> +#nav-bar[brighttext] {

This will also apply on the devedition dark theme, not sure if that's wanted.

Comment 27

3 years ago
(In reply to Tim Nguyen [:ntim] from comment #26)
> Comment on attachment 8659744 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8659744 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/windows/browser.css
> @@ +50,5 @@
> >  
> >    --urlbar-separator-color: hsla(0,0%,16%,.2);
> >  }
> >  
> > +#nav-bar[brighttext] {
> 
> This will also apply on the devedition dark theme, not sure if that's wanted.

Please file a separate bug if it actually ends up not looking as desired.
(Assignee)

Updated

3 years ago
Blocks: 1204615
(Assignee)

Comment 28

3 years ago
Not going to bother uplifting this.
status-firefox42: affected → wontfix
Mozilla/5.0 (Windows NT 10.0; rv:43.0) Gecko/20100101 Firefox/43.0
Verified this fix on Windows 10 32-bit using latest Nightly, build ID: 20150917030229

Filled bug 1206083 and bug 1206087 for some issues spotted while testing this, so we can track them separately.
Status: RESOLVED → VERIFIED
status-firefox43: fixed → verified
(Assignee)

Updated

3 years ago
No longer depends on: 1206087
(Assignee)

Updated

3 years ago
No longer depends on: 1206083
(Assignee)

Updated

3 years ago
Depends on: 1215093
You need to log in before you can comment on or make changes to this bug.