Closed Bug 1173743 Opened 5 years ago Closed 4 years ago

Update toolbar button hover/pressed states on Windows 10

Categories

(Firefox :: Theme, defect, P3)

Unspecified
Windows 10
defect

Tracking

()

VERIFIED FIXED
Firefox 43
Tracking Status
firefox40 + wontfix
firefox41 + wontfix
firefox42 --- wontfix
firefox43 --- verified

People

(Reporter: dao, Assigned: dao)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Blocks: 1173744
Priority: -- → P1
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
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: windows-10
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.
Unfortunately, this is too late for 40. We have time to take a fix for 41 if it is available soon.
-> P3, and a candidate for being in a UI polish clusterbug.
Priority: P2 → P3
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)
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #8657613 - Flags: review?(jaws)
Also, I'm not quite sure if we want this on Windows 8.
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-
(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.
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.
Not targeting 41 then.
No longer blocks: 1192839
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)
Attached patch patch v2Splinter Review
Attachment #8657613 - Attachment is obsolete: true
Attachment #8659744 - Flags: review?(jaws)
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 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+
(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
Closed: 4 years ago
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.
(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.
Blocks: 1204615
Depends on: 1204624
Not going to bother uplifting this.
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
No longer depends on: 1206087
No longer depends on: 1206083
Depends on: 1215093
You need to log in before you can comment on or make changes to this bug.