Closed Bug 1432630 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Theme, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- verified
firefox60 --- verified
firefox61 --- verified

People

(Reporter: mehmet.sahin, Assigned: Kwan)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Attached image 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.
Not intended as far as I know.
Priority: -- → P2
Markus, is this the "rounded corners on Mac sometimes don't"?
Flags: needinfo?(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.
Flags: needinfo?(mstange)
(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.
Flags: needinfo?(moz-ian)
(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.
Assignee: nobody → moz-ian
Blocks: 1422934
Status: NEW → ASSIGNED
Flags: needinfo?(moz-ian)
Summary: Regression?: Tab Close Hover Button look different now → Regression: Tab Close Button lost rounded corners during hover/click
Comment on attachment 8946166 [details]
Bug 1432630 - Restore rounded corners of tab close button.

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

Thanks!
Attachment #8946166 - Flags: review?(dao+bmo) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/834045a73734
Restore rounded corners of tab close button. r=dao
https://hg.mozilla.org/mozilla-central/rev/834045a73734
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1434153
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
Status: RESOLVED → REOPENED
Flags: needinfo?(moz-ian)
Resolution: FIXED → ---
Target Milestone: Firefox 60 → ---
Attachment #8946166 - Attachment is obsolete: true
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 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 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.
Attachment #8946647 - Flags: review?(dao+bmo) → review+
No try run, since it's a minor visual change not currently detectable in automation (to my knowledge).
Flags: needinfo?(moz-ian)
Keywords: checkin-needed
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e68e7c765bdd
Move close-icon rounded corners into SVG. r=dao
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e68e7c765bdd
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
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]: /
Attachment #8946647 - Flags: approval-mozilla-beta?
Comment on attachment 8946647 [details]
Bug 1432630 - Move close-icon rounded corners into SVG.

Small ui fix, looks safe for uplift.
Attachment #8946647 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [good first verify]
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).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.