Closed Bug 1351466 Opened 3 years ago Closed 2 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

53 Branch
x86
Windows 10
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox-esr45 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 - wontfix
firefox54 + wontfix
firefox55 + wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: alice0775, Unassigned)

References

Details

(Keywords: regression, ux-userfeedback)

Attachments

(1 file)

Attached file 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
Flags: needinfo?(masayuki)
Summary: Tabbar would not scroll properly after open bookmarks from "Open All in Tabs". The current tab is not displayed in the tabbar. → Tabbar would not scroll properly after open bookmarks from "Open All in Tabs". Selected tab is not displayed in the tabbar.
What are you asking me with ni?
Flags: needinfo?(masayuki)
(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.
I don't think that we should back out the patches for bug 1320609 because that was very serious bug for some mice users.
BTW, setting toolkit.scrollbox.smoothScroll = false fixes the problem.
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?
Oops, ni? is dropped by mid-air conflict.

dao: please check my previous comment.
Flags: needinfo?(dao+bmo)
(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.
(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.
Flags: needinfo?(dao+bmo)
(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.
Oops, forgot to ni?...
Flags: needinfo?(dao+bmo)
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.
Track 54+/55+ as regression.
Dão, any chance you could check out :masayuki's question in comment 9?
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.
Flags: needinfo?(dao+bmo)
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?
Flags: needinfo?(masayuki)
Flags: needinfo?(dao+bmo)
As I noted in comment 14, it's not clear to me why bug 1320609 broke this. Masayuki, can you look into this?
Flags: needinfo?(dao+bmo)
(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.)
Flags: needinfo?(masayuki)
(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...
Duplicate of this bug: 1383987
Bug 1356705 seems to fix the problem.
Depends on: 1356705
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.