686907 - When opening a new tab, scroll tab strip to show the new tab

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

When the user opens a new tab, on tablets we should scroll the tab strip (animated?) to show the new tab.

Comment 1

[Vishnu S (:vishnus)]

Hi. I would like to work on this... could you help me to get started?

Comment 2

[Martijn Wargers (dead)]

The AddTab function is here:
http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/browser.js#727
I guess on setting the selected tab, the tab should scroll into view:
http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/browser.js#813
Under vbox id="tabs", there is a xul:scrollbox class=tabs-scrollbox
You can perhaps its  boxObject.ensureElementIsVisible() method and use the selected tab element as its argument.

I think that Firefox desktop tabbrowser binding is using something like that.
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#3359

If you want to do it animated, you need to do something more, but I think it's better to first fix it non-animated and handle the animated part in a new bug.

Comment 3

[Vishnu S (:vishnus)]

Ok :) Im currently trying to build the fennec in my laptop. I will message as soon as i get it working. :) Do you use windows as development environment?

Comment 4

[Martijn Wargers (dead)]

Yes. It's better to ask these questions on irc, btw: https://wiki.mozilla.org/IRC#Mozilla.27s_Chat_Server

Comment 5

[Martijn Wargers (dead)]

Mobile developers can be found on the #mobile channel.

Comment 6

[Vishnu S (:vishnus)]

okay cool :)

Comment 7

[Vishnu S (:vishnus)]

hello, i have everything needed to get started. I was looking through some pages in mdn website, I saw Scrolling a child element into view  in https://developer.mozilla.org/en/XUL/scrollbox Is it how it has to be done?

Comment 8

[Vishnu S (:vishnus)]

fixed it! :)

Comment 9

[Vishnu S (:vishnus)]

Attachment: Patch to enable auto scroll to tabstrip to show the new tab.

well, i guess this will fix the issue :) no animation..

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 566146 [details] [diff] [review]
Patch to enable auto scroll to tabstrip to show the new tab.

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

Looks good! Sorry, I didn't realize I was a mentor on this bug even, but looks like martjin helped anyway!

::: mobile/chrome/content/tabs.xml
@@ +254,5 @@
>              this.children.appendChild(tab);
>              this._updateWidth();
> +			let tabListScroller = this.boxObject.QueryInterface(Ci.nsIScrollBoxObject);
> +			tabListScroller.ensureElementIsVisible(tab);
> +            return tab;			

We use spaces instead of tabs, so we'll need to fix that before checkin. There's also some extra whitespace after this return tab call that can be removed.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Let's make sure this doesn't cause problems on the phone UI. If it does, we'll need to do a tablet UI check.

Comment 12

[Vishnu S (:vishnus)]

Attachment: removed extra tabs and spaces.. please check :)

removed extra tabs and spaces (i guess i dint miss any :P ).. please check :)

Comment 13

[Vishnu S (:vishnus)]

i noticed something, if the tab is half visible in the strip, then even if i click on the tab its not scrolling up to show the entire tab. let me try to fix that as well..

Comment 14

[Vishnu S (:vishnus)]

(In reply to Mark Finkle (:mfinkle) from comment #11)
> Let's make sure this doesn't cause problems on the phone UI. If it does,
> we'll need to do a tablet UI check.

I think something is wrong, but as you know the desktop version has some problems right? So is it possible to check it from your side? I dragged page to right in phone UI to show the tab strip, which completely messed up every tabs.

Comment 15

[Josh Matthews [:jdm]]

Wes, is this still a valid bug for a new contributor to be working on? In other words, does this still apply to the Native UI, so that the work done won't be wasted?

Comment 16

[Marco Castelluccio [:marco]]

Does this still apply?

Comment 17

[Wesley Johnston (:wesj)]

No. I'm going to close this bug. This should be fixed in the new native UI.

Comment 18

[Wesley Johnston (:wesj)]

Comment on attachment 567015 [details] [diff] [review]
removed extra tabs and spaces.. please check :)

Clearing this request since this is resolved.