1432630 - Regression: Tab Close Button lost rounded corners during hover/click

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P2)

Description

[Mehmet]

Attachment: FF60_vs_FF58.png

macOS 10.12.6
Nightly 60.0a1 (2018-01-23) (64-Bit)

STR:
1.) Hover over the Tab close button

Result: The corners of the hover button look different between FF58 and FF60. They are angular in FF60.

Not sure if this is intended or a regression?

A screenshot FF60_vs_FF58 is attached.

Comment 1

[Dão Gottwald [:dao]]

Not intended as far as I know.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Markus, is this the "rounded corners on Mac sometimes don't"?

Comment 3

[Markus Stange [:mstange]]

It's not related to bug 1375298, if you mean that.

From a platform perspective, I think this is the correct rendering: The tab close button is a <xul:image> which contains chrome://global/skin/icons/close.svg and applies the hover background using the context-fill property, which affects this element in the SVG:
<path fill="context-fill" fill-opacity="context-fill-opacity" d="M0 0h20v20H0z"/>

This element does not have any rounded corners. I also don't see any border-radius properties set on the image.

Comment 4

[Mehmet]

(In reply to Markus Stange [:mstange] from comment #3)
> It's not related to bug 1375298, if you mean that.
> 
> From a platform perspective, I think this is the correct rendering: The tab
> close button is a <xul:image> which contains
> chrome://global/skin/icons/close.svg and applies the hover background using
> the context-fill property, which affects this element in the SVG:
> <path fill="context-fill" fill-opacity="context-fill-opacity" d="M0
> 0h20v20H0z"/>
> 
> This element does not have any rounded corners. I also don't see any
> border-radius properties set on the image.

Thanks for your feedback. If you take a look at the Mocks at https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/229786513 (Tab Active / Tab Not Active) the hover buttons have rounded corners as they were before this regression.

I did a manual bisect and I found the following regression range:

Good: 2018-01-12-10-01-21

 Bad: 2018-01-12-22-03-34

Maybe caused by https://bugzilla.mozilla.org/show_bug.cgi?id=1422934 ?

Ian Moody: Can you please take a look at the issue? Many thanks in advance.

Comment 5

[Ian Moody [:Kwan] (UTC+0)]

(In reply to Mehmet from comment #4)
> (In reply to Markus Stange [:mstange] from comment #3)
> > It's not related to bug 1375298, if you mean that.
> > 
> > From a platform perspective, I think this is the correct rendering: The tab
> > close button is a <xul:image> which contains
> > chrome://global/skin/icons/close.svg and applies the hover background using
> > the context-fill property, which affects this element in the SVG:
> > <path fill="context-fill" fill-opacity="context-fill-opacity" d="M0
> > 0h20v20H0z"/>
> > 
> > This element does not have any rounded corners. I also don't see any
> > border-radius properties set on the image.
> 
> Thanks for your feedback. If you take a look at the Mocks at
> https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/229786513 (Tab
> Active / Tab Not Active) the hover buttons have rounded corners as they were
> before this regression.
> 
> I did a manual bisect and I found the following regression range:
> 
> Good: 2018-01-12-10-01-21
> 
>  Bad: 2018-01-12-22-03-34
> 
> Maybe caused by https://bugzilla.mozilla.org/show_bug.cgi?id=1422934 ?
> 
> Ian Moody: Can you please take a look at the issue? Many thanks in advance.

Good catch Mehmet!  Definitely caused by that bug.  I didn't even notice (and it doesn't look like screenshots has a test for tab close button hover)
So this is because the border-radius is applied to child elements of .close-icon:
https://searchfox.org/mozilla-central/rev/11d0ff9f36465ce19b0c43d1ecc3025791eeb808/toolkit/themes/shared/close-icon.inc.css#22-28
but in that bug I removed all the child elements and made .close-icon itself the image.
Fix coming up.

Comment 6

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Bug 1432630 - Restore rounded corners of tab close button.

Review commit: https://reviewboard.mozilla.org/r/216160/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216160/

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8946166 [details]
Bug 1432630 - Restore rounded corners of tab close button.

https://reviewboard.mozilla.org/r/216160/#review221978

Thanks!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/834045a73734
Restore rounded corners of tab close button. r=dao

Comment 9

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/834045a73734

Comment 10

[Raul Gurzau (:RaulG)]

Backed out changeset 834045a73734 (bug 1432630) for causing bug 1434153 on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/autoland/rev/89beaa732d7ded3cf2dcb607b7584173df6b96dd

Push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=834045a7373444862b57365d13437c29291860cb

Comment 11

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Bug 1432630 - Move close-icon rounded corners into SVG.

Review commit: https://reviewboard.mozilla.org/r/216630/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216630/

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8946647 [details]
Bug 1432630 - Move close-icon rounded corners into SVG.

https://reviewboard.mozilla.org/r/216630/#review222358

::: toolkit/themes/shared/icons/close.svg:5
(Diff revision 1)
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  <svg xmlns="http://www.w3.org/2000/svg" width="20" height="20">
> -  <path fill="context-fill" fill-opacity="context-fill-opacity" d="M0 0h20v20H0z"/>
> +  <path fill="context-fill" fill-opacity="context-fill-opacity" d="M0 2a2 2 0 0 1 2-2h16a2 2 0 0 1 2 2v16a2 2 0 0 1-2 2H2a2 2 0 0 1-2-2z"/>

perhaps make this a <rect> instead?

Comment 13

[Ian Moody [:Kwan] (UTC+0)]

Comment on attachment 8946647 [details]
Bug 1432630 - Move close-icon rounded corners into SVG.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216630/diff/1-2/

Comment 14

[Ian Moody [:Kwan] (UTC+0)]

Comment on attachment 8946647 [details]
Bug 1432630 - Move close-icon rounded corners into SVG.

https://reviewboard.mozilla.org/r/216630/#review222358

> perhaps make this a <rect> instead?

Done!  (Thanks! Much simpler than groking path syntax)

And thanks to :ntim for the idea to move the rounding into SVG in the first place.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8946647 [details]
Bug 1432630 - Move close-icon rounded corners into SVG.

https://reviewboard.mozilla.org/r/216630/#review222400

::: toolkit/themes/shared/close-icon.inc.css
(Diff revision 2)
>  .close-icon > .button-icon,
>  .close-icon > .button-box > .button-icon,
>  .close-icon > .toolbarbutton-icon {
>    width: 20px;
>    height: 20px;
> -  border-radius: 2px;

This needs to be rebased for the backout.

Comment 16

[Ian Moody [:Kwan] (UTC+0)]

Comment on attachment 8946647 [details]
Bug 1432630 - Move close-icon rounded corners into SVG.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216630/diff/2-3/

Comment 17

[Ian Moody [:Kwan] (UTC+0)]

No try run, since it's a minor visual change not currently detectable in automation (to my knowledge).

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e68e7c765bdd
Move close-icon rounded corners into SVG. r=dao

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 8946647 [details]
Bug 1432630 - Move close-icon rounded corners into SVG.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1422934
[User impact if declined]: Tab close button has sharp corners on hover. It took some time until somebody noticed this since it isn't super-obvious.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not formally yet but I see the fix working in the current Nightly build on Linux
[Needs manual test from QE? If yes, steps to reproduce]: /
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple fix
[String changes made/needed]: /

Comment 21

[Liz Henry (:lizzard)]

Comment on attachment 8946647 [details]
Bug 1432630 - Move close-icon rounded corners into SVG.

Small ui fix, looks safe for uplift.

Comment 22

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cfd2262a3ec5

Comment 23

[Catalin Sasca, Desktop Test Engineering [:csasca]]

I successfully reproduced this issue on Firefox Nightly 60.0a1 (2018-01-23) under macOS 10.12 using STR from Comment 0.

The issue is fixed on Firefox 59.0.2 RC, Firefox 60.0b9 and latest Nightly 61.0a1 (2018-04-03), macOS 10.12. I have also checked on Ubuntu 16.04 (x64) and Windows 10 (x64).