1351466 - Tabbar would not scroll properly after open bookmarks from "Open All in Tabs". Selected tab is not displayed in the tabbar.

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Alice0775 White]

Attachment: about:suppot

[Tracking Requested - why for this release]: UX regression. selected tab is missing in tabbar after doing "Open All in Tabs"


Affected Version: 53.0b7, Aurora54.0a2, Nightly55.0a1
Unaffected Version: 52.0.2

Reproducible: always

Steps To Reproduce:
1. Create a Bookmark folder which contains many bookmarks(about:home 20+)
2. Right mouse click on the folder and select "Open All in Tabs"
3. (if not reproduced) repeat step2 

Actual Results:
Tabbar scrolls to wrong position.
Selected tab is not displayed in the tabbar.

Expected Results:
Selected tab should be displayed and visible in the tabbar.


Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=148e65bd3f3b156d58836dca9fe8055ae5c2c5d1&tochange=9fc13b45970ab17567ff53ff4f34a6a605852707

Regressed by: Bug 1320609

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

What are you asking me with ni?

Comment 2

[Alice0775 White]

(In reply to Masayuki Nakano [:masayuki] from comment #1)
> What are you asking me with ni?

Your patch breaks tabbar scrolling function.

I am asking you that the bunch of patch should be backed out from beta53 and Aurora54.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I don't think that we should back out the patches for bug 1320609 because that was very serious bug for some mice users.

Comment 4

[Alice0775 White]

BTW, setting toolkit.scrollbox.smoothScroll = false fixes the problem.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Looks like that this is not a regression. Bug 1320609 fixed a hidden bug and current behavior may be intentional.

Current behavior is, <scrollbox>.ensureElementIsVisible() is called some times immediately after opening tabs. Then, <scrollbox>._smoothScrollByPixels() is called a lot of times for background tabs.

When you open a lot of tabs from "Open All in Tabs", <tabbrowser>._notifyBackgroundTab() tries to show new background tab:
http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/browser/base/content/tabbrowser.xml#6277,6315

Then, <scrollbox>._smoothScrollByPixels() (although, this is a hidden API...) and its _scrollAnim.start() modify current scroll destination to the new destination. So, if "Open All in Tabs" shouldn't scroll to background tabs, it should set "skipbackgroundnotify" attribute:
http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/browser/base/content/tabbrowser.xml#6412

Or after adding all tabs, <tabbrowser> should call <scrollbox>.ensureElementIsVisible() with selected tab.


dao:

Do you know who is familiar with around this?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, ni? is dropped by mid-air conflict.

dao: please check my previous comment.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Alice0775 White from comment #4)
> BTW, setting toolkit.scrollbox.smoothScroll = false fixes the problem.

Makes sense with this condition:
http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/browser/base/content/tabbrowser.xml#6277,6294,6315

I don't understand why this method tries to scroll to background tab *only* when smooth scroll is enabled, though.

Comment 8

[Dão Gottwald [::dao]]

(In reply to Masayuki Nakano [:masayuki] from comment #7)
> (In reply to Alice0775 White from comment #4)
> > BTW, setting toolkit.scrollbox.smoothScroll = false fixes the problem.
> 
> Makes sense with this condition:
> http://searchfox.org/mozilla-central/rev/
> 7419b368156a6efa24777b21b0e5706be89a9c2f/browser/base/content/tabbrowser.
> xml#6277,6294,6315
> 
> I don't understand why this method tries to scroll to background tab *only*
> when smooth scroll is enabled, though.

See bug 412666 comment 3. Smooth scrolling makes it easier for the user to understand what's happening in the tab strip.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Dão Gottwald [::dao] from comment #8)
> (In reply to Masayuki Nakano [:masayuki] from comment #7)
> > (In reply to Alice0775 White from comment #4)
> > > BTW, setting toolkit.scrollbox.smoothScroll = false fixes the problem.
> > 
> > Makes sense with this condition:
> > http://searchfox.org/mozilla-central/rev/
> > 7419b368156a6efa24777b21b0e5706be89a9c2f/browser/base/content/tabbrowser.
> > xml#6277,6294,6315
> > 
> > I don't understand why this method tries to scroll to background tab *only*
> > when smooth scroll is enabled, though.
> 
> See bug 412666 comment 3. Smooth scrolling makes it easier for the user to
> understand what's happening in the tab strip.

Thanks.

But my original question is different.

According to my investigation mentioned in comment 5, scrolling to the right-most background tabe at "Open All in Tabs" on folder is the intentional behavior with current tabbrowser's code. So, my question is, is it really intentional behavior? or was the previous behavior of bug 1320609 expected but depended on the buggy behavior of <scrollbox>?

If it's the former, this is INVALID.  Otherwise, I don't know where should <tabbrowser> calls ensureElementIsVisible() after all tabs are actually added.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, forgot to ni?...

Comment 11

[Liz Henry (:lizzard)]

Too late for a fix for 53, as we are heading into the last week of the beta 53 cycle and no one is assigned.

Comment 12

[Gerry Chang [:gchang]]

Track 54+/55+ as regression.

Comment 13

[Julien Cristau [:jcristau]]

Dão, any chance you could check out :masayuki's question in comment 9?

Comment 14

[Dão Gottwald [::dao]]

I think this is a valid bug, i.e. the expected behavior is that we scroll background tabs into view, but only so far that the selected tab also stays in view.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #5)
> Looks like that this is not a regression. Bug 1320609 fixed a hidden bug and
> current behavior may be intentional.
> 
> Current behavior is, <scrollbox>.ensureElementIsVisible() is called some
> times immediately after opening tabs. Then,
> <scrollbox>._smoothScrollByPixels() is called a lot of times for background
> tabs.
> 
> When you open a lot of tabs from "Open All in Tabs",
> <tabbrowser>._notifyBackgroundTab() tries to show new background tab:
> http://searchfox.org/mozilla-central/rev/
> 7419b368156a6efa24777b21b0e5706be89a9c2f/browser/base/content/tabbrowser.
> xml#6277,6315
> 
> Then, <scrollbox>._smoothScrollByPixels() (although, this is a hidden
> API...) and its _scrollAnim.start() modify current scroll destination to the
> new destination. So, if "Open All in Tabs" shouldn't scroll to background
> tabs, it should set "skipbackgroundnotify" attribute:
> http://searchfox.org/mozilla-central/rev/
> 7419b368156a6efa24777b21b0e5706be89a9c2f/browser/base/content/tabbrowser.
> xml#6412
> 
> Or after adding all tabs, <tabbrowser> should call
> <scrollbox>.ensureElementIsVisible() with selected tab.

_notifyBackgroundTab already takes the selected tab into account with the intention not to scroll it out of view. Why has this stopped working? tabbrowser should not have to do an extra ensureElementIsVisible call for the selected tab.

Comment 15

[Liz Henry (:lizzard)]

Valid bug, but at this point I think it is going to our general backlog since I'd like to stop triaging it every 6 weeks. 

Masayuki, Dão, last call, either of you want to fix this or find an owner for it?  Is it a priority for anyone?

Comment 16

[Dão Gottwald [::dao]]

As I noted in comment 14, it's not clear to me why bug 1320609 broke this. Masayuki, can you look into this?

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Dão Gottwald [::dao] from comment #16)
> As I noted in comment 14, it's not clear to me why bug 1320609 broke this.

Me too.

> Masayuki, can you look into this?

IIRC, somebody was rewriting the <scrollbox> implementation, isn't it? If it'll be landed soon, it's not worthwhile to this bug now. (And I'm working on some Quantum Flow bugs, I don't think that this is more important than them.)

Comment 18

[Dão Gottwald [::dao]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #17)
> (In reply to Dão Gottwald [::dao] from comment #16)
> > As I noted in comment 14, it's not clear to me why bug 1320609 broke this.
> 
> Me too.
> 
> > Masayuki, can you look into this?
> 
> IIRC, somebody was rewriting the <scrollbox> implementation, isn't it? If
> it'll be landed soon, it's not worthwhile to this bug now. (And I'm working
> on some Quantum Flow bugs, I don't think that this is more important than
> them.)

You probably mean bug 1356705. That's not landing soon, nor is it clear that it would fix this regression of yours...

Comment 20

[Alice0775 White]

Bug 1356705 seems to fix the problem.