Closed Bug 966905 Opened 9 years ago Closed 9 years ago

Headers and footer of subviews should be outside scrollable bits of those subviews

Categories

(Firefox :: Toolbars and Customization, defect)

Product:
Firefox
Component:
Toolbars and Customization
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 30
Tracking Flags:
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: Gijs, Assigned: jaws)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [Australis:P2])

Attachments

(2 files, 4 obsolete files)

Patch v3 (with -w)
10.26 KB, patch
Gijs
: review+
Dolske
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Patch v3
16.16 KB, patch
Details | Diff | Splinter Review 
      No description provided.
Whiteboard: [Australis:P3] → [Australis:P2]
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Summary: Footers of bookmarks/history views should be outside scrollable bits of those panels/subviews → Headers and footer of subviews in the panel should be outside scrollable bits of those subviews
Attached patch Patch (obsolete) — — Splinter Review 

        Attachment #8372633 -
        Flags: review?(gijskruitbosch+bugs)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch (with -w) (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  
Comment on attachment 8372633 [details] [diff] [review]
Patch

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

Clearing review pending investigations about (a) standalone subviews and (b) the height of the subviews when using this patch. Oh, and adding this scrolling thing to the devtools, too.

        Attachment #8372633 -
        Flags: review?(gijskruitbosch+bugs)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v2 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8372633 -
        Attachment is obsolete: true

        Attachment #8372634 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v2 (with -w) (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8372736 -
        Flags: review?(gijskruitbosch+bugs)

    
    

    
  
Comment on attachment 8372739 [details] [diff] [review]
Patch v2 (with -w)

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

::: browser/components/customizableui/content/panelUI.xml
@@ +317,5 @@
> +            let footer = viewNode.querySelector(".panel-subview-footer");
> +            let newHeight = (header ? header.scrollHeight : 0) +
> +                            (body ? body.scrollHeight : 0) +
> +                            (footer ? footer.scrollHeight : 0);
> +                            

I removed this trailing whitespace locally.

    
    

    
  
Comment on attachment 8372739 [details] [diff] [review]
Patch v2 (with -w)

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

f+, because while this generally looks fine, I don't understand the removal of the height setting in combination with the transition listener above it.

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +154,1 @@
>        <vbox id="PanelUI-developerItems"/>

Why not just add this class to the existing vbox?

::: browser/components/customizableui/content/panelUI.xml
@@ -221,5 @@
>            this._viewContainer.addEventListener("transitionend", function trans() {
>              this._viewContainer.removeEventListener("transitionend", trans);
>              this._transitioning = false;
>            }.bind(this));
> -          this._viewContainer.style.height = this._subViews.scrollHeight + "px";

Why get rid of this? Seems to me that now the transition doesn't happen because there's no change to the height of the box? If we're sure that someone else will set the height here, we should have a comment here that indicates this.

Also, while I realize this is minor scope creep, please ensure that the transitioning CSS is in browser/base and not in a shared theme folder.

@@ +312,5 @@
>          <body><![CDATA[
>            if (!this.ignoreMutations && this.showingSubView) {
> +            let viewNode = this._currentSubView;
> +            let header = viewNode.querySelector(".panel-subview-header");
> +            let body = viewNode.querySelector(".panel-subview-body");

Hrm. If body is null, because an add-on or external consumer defined a subview, I think we should use the subview's total scrollheight instead. So I think this should be:

let newHeight = body ? body.scrollHeight : viewNode.scrollHeight;
if (body) {
  let header ...;
  let footer ...;
  newHeight += ...;
}

        Attachment #8372739 -
        Flags: feedback+

    
  

        Attachment #8372736 -
        Flags: review?(gijskruitbosch+bugs)

    
    

    
  


  
The height transition is in /browser/components/customizableui/content/panelUI.css.

I removed the earlier _viewContainer.style.height change because it was working without it on my machine, but that may have just been coincidence. I've added it back.

        Attachment #8372736 -
        Attachment is obsolete: true

        Attachment #8372739 -
        Attachment is obsolete: true

        Attachment #8372852 -
        Flags: review?(gijskruitbosch+bugs)

    
  
Summary: Headers and footer of subviews in the panel should be outside scrollable bits of those subviews → Headers and footer of subviews should be outside scrollable bits of those subviews

    
    

    
  
Comment on attachment 8372852 [details] [diff] [review]
Patch v3 (with -w)

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

Great! Note that this doesn't do anything to the footer of the bookmarks menu when it isn't in the panel. I don't know what, if anything, could be done about that as it's an autoscrolling thing instead of a scrollbar'd panel...

        Attachment #8372852 -
        Flags: review?(gijskruitbosch+bugs) → review+

    
    

    
  
(In reply to :Gijs Kruitbosch from comment #10)
> Comment on attachment 8372852 [details] [diff] [review]
> Patch v3 (with -w)
> 
> Review of attachment 8372852 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great! Note that this doesn't do anything to the footer of the bookmarks
> menu when it isn't in the panel. I don't know what, if anything, could be
> done about that as it's an autoscrolling thing instead of a scrollbar'd
> panel...

Actually, it seems like the most sensible thing to do there would be to just move the item back to the top like it is in the main menu...

Stephen, does that make sense?
Flags: needinfo?(shorlander)

    
    

    
  
(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to :Gijs Kruitbosch from comment #10)
> > Comment on attachment 8372852 [details] [diff] [review]
> > Patch v3 (with -w)
> > 
> > Review of attachment 8372852 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Great! Note that this doesn't do anything to the footer of the bookmarks
> > menu when it isn't in the panel. I don't know what, if anything, could be
> > done about that as it's an autoscrolling thing instead of a scrollbar'd
> > panel...

Yeah, I noticed this too but it wasn't a regression from m-c so I left it alone.

> Actually, it seems like the most sensible thing to do there would be to just
> move the item back to the top like it is in the main menu...
> 
> Stephen, does that make sense?

Even if we do that, it still won't change the scrolling behavior of the panel. I'll go ahead and land this now and we can file a follow-up to move the Show All Bookmarks menuitem if necessary.

    
  
Depends on: 970026

    
  
No longer depends on: 970026

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/0c7bdabe00ce
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30

    
  
Flags: in-testsuite? → in-testsuite-

    
  
Depends on: 970026

    
  
Depends on: 970897

    
    

    
  
Comment on attachment 8372852 [details] [diff] [review]
Patch v3 (with -w)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature, needed for Australis
User impact if declined: footers of subview scroll out of view
Testing completed (on m-c, etc.): on m-c for a while
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

note: needs to be uplifted along with the patch for bug 970897

        Attachment #8372852 -
        Flags: approval-mozilla-aurora?
Flags: needinfo?(shorlander)

    
    

    
  
(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to :Gijs Kruitbosch from comment #10)
> > Comment on attachment 8372852 [details] [diff] [review]
> > Patch v3 (with -w)
> > 
> > Review of attachment 8372852 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Great! Note that this doesn't do anything to the footer of the bookmarks
> > menu when it isn't in the panel. I don't know what, if anything, could be
> > done about that as it's an autoscrolling thing instead of a scrollbar'd
> > panel...
> 
> Actually, it seems like the most sensible thing to do there would be to just
> move the item back to the top like it is in the main menu...
> 
> Stephen, does that make sense?

I think we should be consistent with our new menu system and keep the footer at the bottom. Having it at the top in the old system wasn't ideal either because it puts what is ideally the secondary item in the highest priority slot. We have talked about moving it to the bottom before but doing that in a menu is hard.

If we can't do anything to pin it to the bottom now, we should still leave it at the bottom.

        Attachment #8372852 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 980038

    
  
QA Contact: cornel.ionce

    
  
Depends on: 985024

    
  
Depends on: 986866
Status: RESOLVED → VERIFIED

          status-firefox29:
          fixed → verified

          status-firefox30:
          fixed → verified

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