1376397 - positioning of tabs during repeated tab close with mouse has changed

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Cameron McCormack (:heycam)]

It used to be that if you repeated clicked on a close tab button, that the tabs to the right would slide in so that you could continue clicking in the same location to close the next tab.  That seems to have stopped working recently.

Comment 1

[Cameron McCormack (:heycam)]

This only happens when there are enough tabs to show the scrolling buttons.  mozregression tells me:

19:30.76 INFO: Last good revision: 5f41ffc410de17d74f59770be704f1d08f53d3e0
19:30.76 INFO: First bad revision: ea7d298712442d71a5541fe3692583c6ac1c8815
19:30.76 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5f41ffc410de17d74f59770be704f1d08f53d3e0&tochange=ea7d298712442d71a5541fe3692583c6ac1c8815

Comment 2

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Shouldn't break the UX pattern for closing tabs quickly.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking this for 56, assuming Dão will figure this out or find an owner for the bug after the work week.

Comment 4

[Dão Gottwald [:dao]]

Attachment: Bug 1376397 - Disable scroll buttons if there's space between the first or last element and the scrollbox edge.

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

Comment 5

[Dão Gottwald [:dao]]

The change from == to >= and <= is the essence of the fix. I had to refactor things because RTL would have needed <= where LTR needed >=, which would have made these expressions way more confusing.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8882406 [details]
Bug 1376397 - Disable scroll buttons if there's space between the first or last element and the scrollbox edge.

https://reviewboard.mozilla.org/r/153522/#review158702

r=me with the orient=vertical issue either fixed here (should be using paddingTop and paddingBottom instead for vertical scrollboxes, I think?) or moved to a followup issue. From a casual reading of the patch in bug 1368208, that also regressed with that patch in that scrollPosition used scrollTop/scrollBottom in the vertical case - either that or I'm missing something because it's 1am and I'm sleepy...

::: toolkit/content/widgets/scrollbox.xml:682
(Diff revision 1)
> +                if (this._isRTLScrollbox) {
> +                  [leftOrTopElement, rightOrBottomElement] = [rightOrBottomElement, leftOrTopElement];
> +                }
>  
> -                if (firstElement &&
> -                    Math.round(this._boundsWithoutFlushing(firstElement)[start]) - scrollboxPaddingStart ==
> +                if (leftOrTopElement &&
> +                    leftOrTopEdge(leftOrTopElement) >= leftOrTopEdge(this._scrollbox) + scrollboxPaddingLeft) {

Pre-existing bug with this code, but using the left/right padding on a vertically oriented scrollbox seems wrong? Either fix here or use another followup bug, I guess...

Comment 7

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #6)
> Comment on attachment 8882406 [details]
> Bug 1376397 - Disable scroll buttons if there's space between the first or
> last element and the scrollbox edge.
> 
> https://reviewboard.mozilla.org/r/153522/#review158702
> 
> r=me with the orient=vertical issue either fixed here (should be using
> paddingTop and paddingBottom instead for vertical scrollboxes, I think?) or
> moved to a followup issue. From a casual reading of the patch in bug
> 1368208, that also regressed with that patch in that scrollPosition used
> scrollTop/scrollBottom in the vertical case - either that or I'm missing
> something because it's 1am and I'm sleepy...

It doesn't matter in practice because the only scrollbox using such padding is in the tab strip. I will remove this stuff in bug 1349555.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f8371e8ded2
Disable scroll buttons if there's space between the first or last element and the scrollbox edge. r=Gijs

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6f8371e8ded2

Comment 10

[tabmix.onemen]

When _startEndProps return "top", "bottom" you should use scrollbox padding-top and scrollbox padding-bottom instead of padding-left/right.

Also in vertical orient you don't need to check for _isRTLScrollbox

Comment 11

[:Gijs (he/him)]

(In reply to tabmix.onemen from comment #10)
> When _startEndProps return "top", "bottom" you should use scrollbox
> padding-top and scrollbox padding-bottom instead of padding-left/right.
> 
> Also in vertical orient you don't need to check for _isRTLScrollbox

comment #7 already said that this never happens in practice. Is there something that got missed, and if so, can you be more explicit about what that is?

Comment 12

[tabmix.onemen]

(In reply to :Gijs from comment #11)
> (In reply to tabmix.onemen from comment #10)
> > When _startEndProps return "top", "bottom" you should use scrollbox
> > padding-top and scrollbox padding-bottom instead of padding-left/right.
> > 
> > Also in vertical orient you don't need to check for _isRTLScrollbox
> 
> comment #7 already said that this never happens in practice. Is there
> something that got missed, and if so, can you be more explicit about what
> that is?

Tab mix plus muli-row feature use orient=vertical on scrollbox.
I manage workaround the issue by overwrite the original _updateScrollButtonsDisabledState function

I'm still trying to find a way to keep muli-row feature in Webextension

Comment 13

[tabmix.onemen]

Any one have an idea how to implement muli-row feature in Webextension

Comment 14

[Dão Gottwald [:dao]]

(In reply to tabmix.onemen from comment #13)
> Any one have an idea how to implement muli-row feature in Webextension

I don't. You'll more likely get a response in a webextension-specific channel / forum / mailing list.

Comment 15

[Madhuri]

(In reply to Cameron McCormack (:heycam) from comment #0)
> It used to be that if you repeated clicked on a close tab button, that the
> tabs to the right would slide in so that you could continue clicking in the
> same location to close the next tab.  That seems to have stopped working
> recently.

[bugday-20170726] 

status-ff56.0a1 : VEFIED & NOT FIXED.

Managed to reproduce the issue on Firefox Nightly under Windows 10 X 64.

The issue is still reproducible on Firefox latest Nightly [BuildID : 20170725030209 , 56.0a1(2017-07-25)(32 bit)] with
normal window - closing the tab 33rd time and 19th time in private window.

Comment 16

[Madhuri]

[bugday-20170801] 

status-ff56.0a1 : VERIFIED & NOT FIXED.

Managed to reproduce the issue on Firefox Nightly under Windows 10 X 64.

The issue is still reproducible on Firefox latest Nightly [BuildID : 20170801100311 , 56.0a1 (2017-08-01) (32-bit)] with
normal window - closing the tab 33rd time and 19th time in private window.

Comment 17

[Madhuri]

(In reply to Madhuri from comment #16)
> [bugday-20170801] 
> 
> status-ff56.0a1 : VERIFIED & NOT FIXED.
> 
> Managed to reproduce the issue on Firefox Nightly under Windows 10 X 64.
> 
> The issue is still reproducible on Firefox latest Nightly [BuildID :
> 20170801100311 , 56.0a1 (2017-08-01) (32-bit)] with
> normal window - closing the tab 33rd time and 19th time in private window.


[bugday-20170802]  // due to updates on etherpad link.

Comment 18

[Madhuri]

[bugday-20170809] 

status-ff57.0a1 : VERIFIED & NOT FIXED.

Managed to reproduce the issue on Firefox Nightly under Windows 10 X 64.

The issue is still reproducible on Firefox latest Nightly [BuildID : 20170808114032 , 57.0a1 (2017-08-08) (32-bit)] with
normal window - closing the tab on 17th time and 14th time in private window.

Comment 19

[Madhuri]

[bugday-20170823] 

status-ff57.0a1 : VERIFIED & NOT FIXED.

Managed to reproduce the issue on Firefox Nightly under Windows 10 X 64.

The issue is still reproducible on Firefox latest Nightly [BuildID : 20170823100553 , 57.0a1 (2017-08-23) (32-bit)] with
normal window - closing the tab on 25th time and 21st time in private window.

Comment 20

[alex_mayorga]

¡Hola!

Reopening per comment 15 and onward.

This is still a problem in https://hg.mozilla.org/mozilla-central/rev/d10c97627b51a226e19d0fa801201897fe1932f6

Once the tab bar is overflown clicking the X repeatedly without moving the cursor ends up clicking on the +

¡Gracias!
Alex

Comment 21

[Dão Gottwald [:dao]]

Pretty sure the patch worked when it landed, and as long as that patch hasn't been backed out, we should keep this bug closed. There are two likely scenarios: 1) The patch only fixed part of the problem, 2) something else regressed this again. Either way, we should file a new bug on that.

Comment 22

[alex_mayorga]

¡Hola Dão!

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1396911 FWIW.

¡Gracias!
Alex