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

RESOLVED FIXED in Firefox 53

Status

()

defect
--
minor
RESOLVED FIXED
10 years ago
2 years ago

People

(Reporter: thatiparthysreenivas, Assigned: premangvikani)

Tracking

(Depends on 1 bug)

3.5 Branch
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [FFT3.5])

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

10 years ago
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 .
Reporter

Updated

10 years ago
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.
Reporter

Updated

10 years ago
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
Posted image screenshot
Resizing the window for a couple of pixels solves it.
Assignee

Comment 2

3 years ago
I would like to work on this Bug, can I please?
Assignee

Updated

3 years ago
Flags: needinfo?(dao+bmo)
Assignee

Comment 3

3 years ago
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)
Assignee

Updated

3 years ago
Flags: needinfo?(dao+bmo)
Assignee

Comment 4

3 years ago
Posted 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-
Posted 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)
Assignee

Comment 10

3 years ago
(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..
Assignee

Comment 11

3 years ago
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)
Assignee

Comment 12

3 years ago
Posted 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+
Assignee

Comment 14

3 years ago
Posted 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
Assignee

Comment 16

3 years ago
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)
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 18

3 years ago
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

Comment 19

3 years ago
bugherder
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]

Updated

2 years ago
Depends on: 1347382

Updated

2 years ago
Depends on: 1370099
You need to log in before you can comment on or make changes to this bug.