875894 - New tab button clip-path clips the image's stroke

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

Attachment: Screenshot of new tab button on OS X HiDPI

STR:
1) Use DOMi to force the :hover pseudo-class on .tabs-newtab-button when there isn't tab overflow
2) Delete the clip-path property applied to .tabs-newtab-button

Expected result:
No visual difference

Actual result:
More of the stroke in the hover image is shown

This is very noticeable with OS X HiDPI where the bottom (non-rectangluar) part of the clip-path clips a lot of the stroke and it looks quite bad.

Two options:
1) Make the clip-path curves wider than so it only clips transparent pixels.
2) Move the clip-path to a pseudo-element with pointer-events: auto and set pointer-events: none on .tabs-newtab-button. This is similar to how we use the clip-path on background tabs where the clip-path is on ::before and the stroke is in ::after. This approach means we can have an accurate clickable region without clipping the image. The downside is that it adds more pseudo-elements but in this case it would only be used for the clip-path so probably wouldn't need to be poked at as much.

Comment 1

[Matthew N. [:MattN]]

Attachment: v.1 Option 1 - Make new tab button clip-path wider

(This patches applies on top of bug 875326)
This implements option 1 to simply change the clipPath. I personally find that it makes the button's hover target overlap the last tab too much (on OS X).

I know people don't like pseudo-elements and the bugs they cause (e.g. bug 853415) but I'm not sure if this larger clip-path is acceptable.

Option 3: Move the clip-path to the image inside the button. I couldn't get the pointer-events to work with pointer-events: none on the button and pointer-events: auto on the image (similar to what we do with tabs).

Comment 2

[:Gijs (he/him)]

Comment on attachment 756410 [details] [diff] [review]
v.1 Option 1 - Make new tab button clip-path wider

(In reply to Matthew N. [:MattN] from comment #1)
> Created attachment 756410 [details] [diff] [review]
> v.1 Option 1 - Make new tab button clip-path wider
> 
> (This patches applies on top of bug 875326)
> This implements option 1 to simply change the clipPath. I personally find
> that it makes the button's hover target overlap the last tab too much (on OS
> X).

Can we fix this overlap with z-index in some way, so it's positioned "below" the other tab? Otherwise, this path looks fine to me.

Comment 3

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #2)
> Comment on attachment 756410 [details] [diff] [review]
> v.1 Option 1 - Make new tab button clip-path wider
> 
> (In reply to Matthew N. [:MattN] from comment #1)
> > Created attachment 756410 [details] [diff] [review]
> > v.1 Option 1 - Make new tab button clip-path wider
> > 
> > (This patches applies on top of bug 875326)
> > This implements option 1 to simply change the clipPath. I personally find
> > that it makes the button's hover target overlap the last tab too much (on OS
> > X).
> 
> Can we fix this overlap with z-index in some way, so it's positioned "below"
> the other tab? Otherwise, this path looks fine to me.

Thanks for the quick review.

To be clear, it's not a visual overlap, just for the pointer-events.

With some quick tests it seems like it's not too hard but: 
1) It would require positioning and setting the the z-index of the last visible tab if it's not selected. I don't know how that affects layout performance or what other problems that would cause but it's something we could try.
2) It would mean we favour the last visible tab over the new tab button in terms of hit area which I think is the wrong balance since I suspect the new tab button is more commonly clicked.

I'll let UX decide whether the clickable/hoverable area overlaps the last tab too much.

Comment 4

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #3)
> (In reply to :Gijs Kruitbosch from comment #2)
> > Can we fix this overlap with z-index in some way, so it's positioned "below"
> > the other tab? Otherwise, this path looks fine to me.
> 
> Thanks for the quick review.
> 
> To be clear, it's not a visual overlap, just for the pointer-events.

Good, that's what I thought.
 
> With some quick tests it seems like it's not too hard but: 
> 1) It would require positioning and setting the the z-index of the last
> visible tab if it's not selected. I don't know how that affects layout
> performance or what other problems that would cause but it's something we
> could try.
> 2) It would mean we favour the last visible tab over the new tab button in
> terms of hit area which I think is the wrong balance since I suspect the new
> tab button is more commonly clicked.
> 
> I'll let UX decide whether the clickable/hoverable area overlaps the last
> tab too much.

+1 both on leaning towards not doing it, and on letting UX decide, then. :-)

Comment 5

[Stephen Horlander [:shorlander]]

Comment on attachment 756410 [details] [diff] [review]
v.1 Option 1 - Make new tab button clip-path wider

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

This approach sounds ok to me.

Comment 6

[Matthew N. [:MattN]]

Attachment: v.1 Option 1 - Screenshot

Comment 7

[Matthew N. [:MattN]]

Thanks.

https://hg.mozilla.org/projects/ux/rev/bb94dac2a65c

Comment 8

[:Gijs (he/him)]

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