positioning of tabs during repeated tab close with mouse has changed

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
6 months ago
13 days ago

People

(Reporter: heycam, Assigned: dao)

Tracking

({regression})

56 Branch
Firefox 56
regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56+ fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

6 months ago
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.
(Reporter)

Comment 1

6 months ago
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
Blocks: 1368208
Keywords: regression
(Assignee)

Updated

6 months ago
Flags: needinfo?(dao+bmo)

Comment 2

6 months ago
[Tracking Requested - why for this release]:
Shouldn't break the UX pattern for closing tabs quickly.
status-firefox56: --- → affected
tracking-firefox56: --- → ?

Updated

6 months ago
status-firefox54: --- → unaffected
status-firefox55: --- → unaffected
Tracking this for 56, assuming Dão will figure this out or find an owner for the bug after the work week.
tracking-firefox56: ? → +
Comment hidden (mozreview-request)
(Assignee)

Comment 5

6 months ago
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.
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(dao+bmo)

Comment 6

6 months ago
mozreview-review
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...
Attachment #8882406 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 7

6 months ago
(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

6 months ago
Pushed by dgottwald@mozilla.com:
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

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6f8371e8ded2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Comment 10

5 months ago
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

5 months ago
(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?
Flags: needinfo?(tabmix.onemen)

Comment 12

5 months ago
(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
status-firefox-esr52: --- → unaffected

Comment 13

5 months ago
Any one have an idea how to implement muli-row feature in Webextension
(Assignee)

Comment 14

5 months ago
(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

5 months ago
(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

5 months ago
[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

4 months ago
(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

4 months ago
[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

4 months ago
[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

4 months ago
¡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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 21

3 months ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 6 months ago3 months ago
Flags: needinfo?(tabmix.onemen)
Resolution: --- → FIXED

Updated

3 months ago
See Also: → bug 1396911

Comment 22

3 months ago
¡Hola Dão!

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

¡Gracias!
Alex

Updated

13 days ago
Version: 52 Branch → 56 Branch
You need to log in before you can comment on or make changes to this bug.