Closed Bug 498976 Opened 11 years ago Closed 3 years ago

Clicking on active tab doesn't bring it back into view (close button hidden) when scroll arrows were used and tab is halfway hidden

Categories

(Firefox :: Tabbed Browser, defect)

3.5 Branch
defect
Not set
minor

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: thatiparthysreenivas, Assigned: premangvikani)

References

(Depends on 1 open bug)

Details

(Whiteboard: [FFT3.5])

Attachments

(3 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1) Gecko/20090616 Firefox/3.5 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1) Gecko/20090616 Firefox/3.5 (.NET CLR 3.5.30729)

Open as many tabs as possible so that you can see left scroll arrow and right scroll  arrow buttons.Now select any tab,click on the left scroll to send selected  tab to right and it should be half way through the right scroll ,so that u cant see close(x) mark,if you click again on the selected tab ,you don't get the close(x) mark to close that particular tab.ex: Open 11 tabs,select 2 nd one from right most and click on left scroll arrow to make it half way through of right scroll arrow.Then click again on the selected tab.You will get behaviour as explained above.You can  reproduce this for no of tabs >11 situation.

Reproducible: Always

Steps to Reproduce:
1.Open as many tabs as possible so that you can see left scroll arrow and right scroll  arrow buttons( no of tabs >11 )
2.Now select any tab,click on the left scroll to send selected  tab to right and it should be half way through the right scroll ,so that u cant see close(x) mark.
3.you click again on the selected tab
Actual Results:  
the selected tab is not scrolling left to show the close(x) mark.

Expected Results:  
the selected tab should be scrolled left to show the close(x) mark

No themes installed .
Summary: selected tab is not scrolled back when it selected again to see close(x) button to close it, while it is halfway through right arrow. → selected tab is not scrolled back when it is selected again to see close(x) button to close it, while it is halfway through right arrow.
Summary: selected tab is not scrolled back when it is selected again to see close(x) button to close it, while it is halfway through right arrow. → selected tab is not scrolled back when it is selected again to see close(x) button to close it, while it is halfway through right scroll arrow.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
Summary: selected tab is not scrolled back when it is selected again to see close(x) button to close it, while it is halfway through right scroll arrow. → Clicking on active tab doesn't bring it back into view (close button hidden) when scroll arrows were used and tab is halfway hidden
Whiteboard: [FFT3.5]
Version: unspecified → 3.5 Branch
Attached image screenshot
Resizing the window for a couple of pixels solves it.
I would like to work on this Bug, can I please?
Flags: needinfo?(dao+bmo)
Is it ok to scroll to right when user hover the mouse pointer on partially visible tab, in order to make the tab fully visible?
Flags: needinfo?(dao+bmo)
Flags: needinfo?(dao+bmo)
Attached patch changes.patch (obsolete) — Splinter Review
I've done changes to reflect the idea I proposed. Please review it.
Attachment #8825179 - Flags: review?(dao+bmo)
Comment on attachment 8825179 [details] [diff] [review]
changes.patch

Need to get ui-review first.
Flags: needinfo?(dao+bmo)
Attachment #8825179 - Flags: review?(dao+bmo) → ui-review?(ux-review)
Comment on attachment 8825179 [details] [diff] [review]
changes.patch

This works well and will be a nice improvement. The one change we should make though is to make the new animation match the other tab animation. I'll attach a video that shows the difference but basically this new animation is more of a pop where the animation when you click on a tab that is half-visible but not active has a bit of easing to it. I assume the values for the animation for that code can be used here.
Attachment #8825179 - Flags: ui-review?(mverdi) → ui-review-
Attached video tabanimation.mp4
The first part of this video shows the new interaction where the tab pops into place. After the fade to black is an example of the animation for a similar interaction. There the tab kind of slides into place.
Comment on attachment 8825179 [details] [diff] [review]
changes.patch

>+          if(this.selected)

nit: space after 'if'

>+            tabContainer._handleTabSelect(false);

Calling _handleTabSelect without 'false' as the argument should enable the animation that Verdi was talking about.

Would you like to update the patch?
Flags: needinfo?(premangvikani)
(In reply to Dão Gottwald [:dao] from comment #9)
> Comment on attachment 8825179 [details] [diff] [review]
> changes.patch
> 
> >+          if(this.selected)
> 
> nit: space after 'if'
> 
> >+            tabContainer._handleTabSelect(false);
> 
> Calling _handleTabSelect without 'false' as the argument should enable the
> animation that Verdi was talking about.
> 
> Would you like to update the patch?

Yes I would like to update..
Comment on attachment 8825179 [details] [diff] [review]
changes.patch

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -7011,6 +7011,10 @@
>           let tabContainer = this.parentNode;
>           let visibleTabs = tabContainer.tabbrowser.visibleTabs;
>           let tabIndex = visibleTabs.indexOf(this);
>+
>+          if (this.selected)
>+            tabContainer._handleTabSelect();
>+          
>           if (tabIndex == 0) {
>             tabContainer._beforeHoveredTab = null;
>           } else {
Attachment #8825179 - Flags: review?(dao+bmo)
Attached patch Edited patch as suggested. (obsolete) — Splinter Review
I edited the patch as you suggested and now it shows the animation as wanted.
Attachment #8827753 - Flags: review?(dao+bmo)
Attachment #8825179 - Attachment is obsolete: true
Attachment #8825179 - Flags: review?(dao+bmo)
Comment on attachment 8827753 [details] [diff] [review]
Edited patch as suggested.

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -7011,6 +7011,10 @@
>           let tabContainer = this.parentNode;
>           let visibleTabs = tabContainer.tabbrowser.visibleTabs;
>           let tabIndex = visibleTabs.indexOf(this);
>+
>+          if (this.selected)
>+            tabContainer._handleTabSelect();
>+          

Looks good, but the seemingly empty line at the bottom has a bunch of trailing spaces. Please remove these.

Thanks!
Attachment #8827753 - Flags: review?(dao+bmo) → review+
Attached patch Final patchSplinter Review
Extra spaces removed.
Flags: needinfo?(premangvikani)
Attachment #8827970 - Flags: review?(dao+bmo)
Attachment #8827753 - Attachment is obsolete: true
Comment on attachment 8827970 [details] [diff] [review]
Final patch

Thanks!
Attachment #8827970 - Flags: review?(dao+bmo) → review+
Assignee: nobody → premangvikani
Dao, can you please do try build for the patch? And if not required then please tell what am I supposed to do next for this bug? Should I add checkin-needed keyword? or something else?
Flags: needinfo?(dao+bmo)
(In reply to Premang from comment #16)
> Should I add checkin-needed keyword? or something else?

Yes. I would have landed this earlier if the tree hadn't been closed.
Flags: needinfo?(dao+bmo)
Keywords: checkin-needed
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02c94a326f6d
Scroll the selected tab into view on mouseover when it's only partially visible. r=dao
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/02c94a326f6d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
QA Whiteboard: [good first verify]
Depends on: 1347382
Depends on: 1370099
You need to log in before you can comment on or make changes to this bug.