1362281 - Forward button looks different

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P2)

Description

[Markus Stange [:mstange]]

Attachment: Screen Shot 2017-05-04 at 11.43.33 PM.png

[Tracking Requested - why for this release]:

See the screenshot. I think this looked different before, so it's probably an unintended regression.

Comment 1

[Johann Hofmann [:johannh]]

This was caused by consolidating the button styling to use the Windows style. The urlbar style hasn't been shared and that's conflicting a bit, I guess. We should probably find a way to make the button the same height and remove the top border again.

Comment 2

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #1)
> This was caused by consolidating the button styling to use the Windows
> style. The urlbar style hasn't been shared and that's conflicting a bit, I
> guess. We should probably find a way to make the button the same height and
> remove the top border again.

It technically has the same height, the location bar just has an outer box shadow.

I think we should wontfix this. This button in its current form is going away in Firefox 57, and the different look should be acceptable for two release cycles, so it's not worth spending more time on this.

Comment 3

[Markus Stange [:mstange]]

I think the other reason that this change is so noticeable is that the back button is less opaque when disabled than before.
Should I file a new bug for that?

(In reply to Dão Gottwald [::dao] from comment #2)
> and the different look should be acceptable for two
> release cycles

Shipping unintended visuals for two release cycles doesn't sound very desirable to me. Is it feasible to back out the changes that caused this on Beta?

Comment 4

[Dão Gottwald [:dao]]

(In reply to Markus Stange [:mstange] from comment #3)
> I think the other reason that this change is so noticeable is that the back
> button is less opaque when disabled than before.
> Should I file a new bug for that?

Feel free to file a bug, but this is now consistent with what we've been doing on Windows, so this might be wontfix too.

> (In reply to Dão Gottwald [::dao] from comment #2)
> > and the different look should be acceptable for two
> > release cycles
> 
> Shipping unintended visuals for two release cycles doesn't sound very
> desirable to me.

Certainly not desirable, but it's a trade-off that I think is reasonable. FWIW, Johann and I were aware of this change when Johann worked on this patch and deemed it acceptable. If course, we could be wrong. Stephen?

> Is it feasible to back out the changes that caused this on
> Beta?

No, it's too big a change and we're already making further changes on top of it.

Comment 5

[Markus Stange [:mstange]]

(In reply to Dão Gottwald [::dao] from comment #4)
> (In reply to Markus Stange [:mstange] from comment #3)
> > I think the other reason that this change is so noticeable is that the back
> > button is less opaque when disabled than before.
> > Should I file a new bug for that?
> 
> Feel free to file a bug, but this is now consistent with what we've been
> doing on Windows, so this might be wontfix too.

Filed bug 1362408.

Comment 6

[Dão Gottwald [:dao]]

Not part of photon even if we might end up doing something about this for Firefox 55.

Comment 7

[Stephen Horlander [:shorlander]]

Attachment: fix-forward-button-macos.patch

It's weird enough that it's probably worth fixing. This approach seems to work.

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8865581 [details] [diff] [review]
fix-forward-button-macos.patch

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

Works for me as a temporary solution until we throw it out 57.

Thanks Stephen!

Comment 9

[Johann Hofmann [:johannh]]

I'll land it for you if you like.

Comment 10

[Johann Hofmann [:johannh]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c6ae95fa023104532bb8fe8f0813fe8a6fa126
Bug 1362281 - Fix forward button styling for macOS. r=johannh

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8865581 [details] [diff] [review]
fix-forward-button-macos.patch

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

::: browser/themes/osx/browser.css
@@ +587,5 @@
> +  #forward-button > .toolbarbutton-icon {
> +    border-top: none !important;
> +    border-bottom: none !important;
> +    box-shadow: none !important;
> +    box-shadow: 0 .5px 0 0 rgba(0,0,0,0.2) !important;

box-shadow: none !important; can be removed.

Comment 12

[Johann Hofmann [:johannh]]

Good point.

Comment 13

[Johann Hofmann [:johannh]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe0b02d865c6d02e8f5eb238c19e5ad3fb40c18
Bug 1362281 followup - Remove redundant CSS rule. r=me

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d7c6ae95fa02
https://hg.mozilla.org/mozilla-central/rev/7fe0b02d865c