Closed Bug 966905 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
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)

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: 6 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?
(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
You need to log in before you can comment on or make changes to this bug.