Closed Bug 936562 Opened 11 years ago Closed 11 years ago

Change - When opening a new background tab, new tab should be visible and just to the right of parent/sibling tabs

Categories

(Firefox for Metro Graveyard :: Browser, defect, P2)

Product:
Firefox for Metro Graveyard
Component:
Browser
Platform:
All
Windows 8.1
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
Firefox 28

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

(Keywords: platform-parity, polish, ux-discovery, Whiteboard: [block28] feature=change c=tbd u=tbd p=1)

Attachments

(1 file)

patch
6.34 KB, patch
sfoster
: review+
Details | Diff | Splinter Review 
Steps to reproduce:
1) In Metro Firefox, open enough tabs to fill the tab strip.
2) View a web page in the first (leftmost) tab.
3) Right-click on a link and choose "Open in new tab."

Desired results: The new tab opens immediately to the right of the selected tab, and the tab strip scrolls itself if necessary to make the new tab and the selected tab both visible.  (This is how desktop Firefox behaves.)

Actual results: The new tab opens to the right of *all* the tabs and is not visible.  The tab strip does not scroll itself.  It looks like nothing happened.
Blocks: metrov1it19
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: p=1 → [block28] feature=change c=tbd u=tbd p=1
Attached patch patch — — Splinter Review 

        Attachment #829443 -
        Flags: review?(sfoster)

    
    

    
  
Note: The patch matches default desktop Firefox behavior, where if you open multiple new tabs with the same "parent" then each new tab is places right after its "siblings".
Summary: Change - When opening a new background tab, tab strip should scroll to show both current and new tab → Change - When opening a new background tab, new tab should be visible and just to the right of parent/sibling tabs

    
    

    
  
Comment on attachment 829443 [details] [diff] [review]
patch

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

Looks good, works well. Yummy docs.

::: browser/metro/base/content/bindings/tabs.xml
@@ +163,4 @@
>          <body>
>            <![CDATA[
>              let tab = document.createElement("documenttab");
> +            if (aIndex >= 0) {

arguable nit: Maybe guard against bad input with aIndex >= 0 && aIndex < this.strip.childElementCount ?

        Attachment #829443 -
        Flags: review?(sfoster) → review+

    
    

    
  
(In reply to Sam Foster [:sfoster] from comment #3)
> arguable nit: Maybe guard against bad input with aIndex >= 0 && aIndex <
> this.strip.childElementCount ?

The code is actually robust against bad input (it'll just append to the end if there's no element with that index), so I left out the check on the grounds it is redundant, though I can see the argument for it...

https://hg.mozilla.org/integration/fx-team/rev/2eb006e8274f

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/2eb006e8274f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28

    
    

    
  
Verified as fixed, for iteration #19, using latest Nightly from 2013-12-02, on both Win 8 32-bit and 64-bit, by following the STR from comment 0.
Status: RESOLVED → VERIFIED
OS: Windows 8 Metro → Windows 8.1

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: