933674 - Overlapping portion (15px) of the selected tab consumes clicks intended for the adjacent background tab

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

The pointer-events are no longer clipped to the curve shape since bug 921038 landed. The clip-path via CSS was the main performance bottleneck so we'll have to either:

A) live with rectangular clickable regions but fix them to only be 15px wide instead of 30px. This is what we always do for background tabs. See patch coming up which is a trivial 4 line deletion and is easy to revert if we get to plan C below.

B) only use the proper curve clip-path on the selected tab when we're not animating, presumably :not([fadein]), assuming that this doesn't regress perf. This will likely be mostly selector changes.

C) If bug 914617 lands and perf is good, we can backout bug 921038 and be back to the tabs we've been testing for months.

I would like to get to C if the timeline works out but I think we should do A or B or some alternative fix for now.

Marking as P1 because we can't ship with the close button of the tab to the left of the selected tab being partially unclickable.

Comment 1

[Matthew N. [:MattN]]

Attachment: Approach A v.1 - only 15px rect. overlap like bg. tabs

Comment 2

[Matthew N. [:MattN]]

Attachment: Approach B WIP1 - use the CSS clip-path when not animating

UX Base (9d0305b1dd4c): https://tbpl.mozilla.org/?tree=Try&rev=ac54cc3f3ca4
Try push of patch: https://tbpl.mozilla.org/?tree=Try&rev=47931ef0b1f3

Note that this only differs from Approach A in function for the bottom tail of the selected tab's curves that overlap background tabs.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 825800 [details] [diff] [review]
Approach A v.1 - only 15px rect. overlap like bg. tabs

(In reply to Matthew N. [:MattN] from comment #2)
> Created attachment 825822 [details] [diff] [review]
> Approach B WIP1 - use the CSS clip-path when not animating
> 
> UX Base (9d0305b1dd4c): https://tbpl.mozilla.org/?tree=Try&rev=ac54cc3f3ca4
> Try push of patch: https://tbpl.mozilla.org/?tree=Try&rev=47931ef0b1f3

http://compare-talos.mattn.ca/?oldRevs=ac54cc3f3ca4&newRev=47931ef0b1f3&submit=true

Unless I messed something up, approach B does seem to bring back the XP TART regression that bug 921038 fixed so I propose that we go with approach A at least until approach C is ready. Seth tells me bug 914617 is high priority for him.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 825822 [details] [diff] [review]
Approach B WIP1 - use the CSS clip-path when not animating

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

::: browser/themes/shared/tabs.inc.css
@@ +163,5 @@
>    background-size: 100% 100%;
>  }
>  
> +/* Clip the pointer-events of the start and end outside of animations only (for perf.) */
> +.tab-background-start[selected=true]:-moz-locale-dir(ltr):not([fadein])::before,

I don't believe .tab-background-start or .tab-background-end xbl:inherit the fadein attribute. I'll re-push with this change.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Oh, another reason - fadein is no good as an attribute to detect animating. We apply fadein="true" to trigger the animation. We apply fadein="false" to skip the animation. So a tab with selected="true" and not:([fadein]) is not possible.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 825800 [details] [diff] [review]
Approach A v.1 - only 15px rect. overlap like bg. tabs

I think this is fine for now.

When we get clip-path caching, we should put this stuff back. Can you file a bug on that?

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 825800 [details] [diff] [review]
Approach A v.1 - only 15px rect. overlap like bg. tabs

Landed approach A: https://hg.mozilla.org/projects/ux/rev/d62709c0f723 This will at least stop the regular dupes from coming in.

I filed bug 934915 for the proper solution after bug 914617.

(In reply to Mike Conley (:mconley) from comment #6)
> Comment on attachment 825822 [details] [diff] [review]
> Approach B WIP1 - use the CSS clip-path when not animating
> 
> ::: browser/themes/shared/tabs.inc.css
> @@ +163,5 @@
> >    background-size: 100% 100%;
> >  }
> >  
> > +/* Clip the pointer-events of the start and end outside of animations only (for perf.) */
> > +.tab-background-start[selected=true]:-moz-locale-dir(ltr):not([fadein])::before,
> 
> I don't believe .tab-background-start or .tab-background-end xbl:inherit the
> fadein attribute. I'll re-push with this change.

Yeah, that's true and I even checked that in tabbrowser.xml while writing the patch but I somehow forgot in the 3am haze. The fact that I didn't verify that with manual testing was partly why I left WIP on the patch name.

Comment 11

[Dave Townsend [:mossop]]

This fix is so awesome

Comment 12

[:Gijs (he/him)]

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