Closed Bug 1184447 Opened 5 years ago Closed 5 years ago

Control Center door hanger's "More Information" is not clickable on bottom half when tracking elements are present

Categories

(Firefox :: General, defect, P1)

defect
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 42
Iteration:
42.3 - Aug 10
Tracking Status
firefox41 --- unaffected
firefox42 --- verified

People

(Reporter: cpeterson, Assigned: bgrins)

Details

(Whiteboard: [fxprivacy] [campaign])

Attachments

(2 files, 1 obsolete file)

STR:
1. With Tracking Protection enabled, load nytimes.com or irccloud.mozilla.com.
2. You will see two Tracking Protection shield icons (bug 1184312).
3. Click on the page info icon to open the Control Center.
4. Try to click the *bottom half* of the "More Information" button, i.e. below the "More Information" text.

RESULT:
Nothing happens when you click below the "More Information" text. This only happens on pages that have two Tracking Protection shield icons. On most pages, the "More Information" button is clickable everywhere.
No longer blocks: 1184312
Flags: qe-verify+
Flags: firefox-backlog?
Whiteboard: [fxprivacy]
Don't have a great idea off-hand but that seems to be some XUL weirdness here...
Flags: firefox-backlog? → firefox-backlog+
Looks like this is somehow caused by the TP paragraphs, but only if they wrap around. If you go to a site that doesn't have trackers the "More Information" button is fine. Those wrapping labels seem to confuse XUL or rendering or whatever.
Hi Tim, is this bug necessary for the V1 release?  If so, it can be set to V1.  If not, can you set it to P2 - P4.
Flags: needinfo?(ttaubert)
Yeah, we unfortunately need to fix that for V1.
Flags: needinfo?(ttaubert)
Estimating conservatively here, hoping it's a lot less effort.
Points: --- → 5
Rank: 1
Priority: -- → P1
QA Contact: mwobensmith
It looks like the height on panel-viewcontainer is wrong, and clipping off the bottom of the button when the paragraph is visible
I'm guessing _setViewContainerHeight is maybe being called at the wrong time (possibly because the display on the labels changes after the panel is shown).  So we may need to force the panel to relayout after  we change the state of the labels: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-trackingprotection.js#49.
Summary: Control Center door hanger's "More Information" is not clickable on bottom half (when two Tracking Protection shield icons are shown) → Control Center door hanger's "More Information" is not clickable on bottom half when tracking elements are present
I (In reply to Brian Grinstead [:bgrins] from comment #6)
> It looks like the height on panel-viewcontainer is wrong, and clipping off
> the bottom of the button when the paragraph is visible

That's not what's happening if you inspect the panel. You can adjust the height and the panel will grow. I think it's an error in the XUL layout code somewhere.
(In reply to Tim Taubert [:ttaubert] from comment #8)
> I (In reply to Brian Grinstead [:bgrins] from comment #6)
> > It looks like the height on panel-viewcontainer is wrong, and clipping off
> > the bottom of the button when the paragraph is visible
> 
> That's not what's happening if you inspect the panel. You can adjust the
> height and the panel will grow. I think it's an error in the XUL layout code
> somewhere.

The really weird thing is that if I set noautohide='true' on the panel and then inspect the #identity-popup-mainView in the Browser Toolbox (but don't change anything) then the button working as normally...
That's weird, with noautohide=true I can inspect the failing state and still not click the lower half of the button. If I set max-height:0 on the visible TP <label> then it will still draw the whole text but only occupy as much height as the layout thinks it should - and the button is clickable again.
Interestingly #tracking-protection-content seems to be overflowing - if I set overflow: hidden then it clips off part of the 'enable protection' button and the full 'more info' button is clickable again.  Seems like some xul weirdness with the wrapping label.
Seems that the 2 line label is 30px tall (as confirmed by getBoundingClientRect), but if you wrap it in a vbox and then set overflow:hidden on the vbox then it clips the label to one line (~15px).  It seems like the label is somehow not actually reserving the full 30px.  I've tried a ton of combinations of display: block, overflow: hidden, overflow: -moz-hidden-unscrollable[0], min/max-height, position:relative, other styles, and a variety of nesting elements and couldn't yet figure out a way to fix it in CSS.

I believe running this in JS would work:  
label.style.height = label.clientHeight + "px";

But it's pretty nasty b/c you can't run that code when it's hidden, otherwise label.style.height = "0px" and then the next call to clientHeight will return 0.

[0]: http://codeverge.com/mozilla.dev.tech.xul/overflow/1552524
Could possibly be related to Bug 353063 or Bug 228673
Attached file label-overflow.xul
reduced xul testcase showing the problem
roc, does this ring a bell? Is this a known problem and hard to fix? It seems like a wrapping <xul:label> doesn't correctly propagate its height after wrapping, although the layout looks correct. It might also help if you had a few hints and places that we could look at for debugging.
Flags: needinfo?(roc)
This will btw get only worse with more wrapping labels... Currently working on bug 1175702 that does add one more label.
I haven't looked into this in great detail, but generally XUL box layout is bad at handling wrapped content. CSS flexbox is much better about it. Can you use that instead?
Flags: needinfo?(roc)
Removing this "tiny-hack" does seem to resolve this issue:
#identity-popup,
.panel-viewstack[viewtype="main"]:not([transitioning]) > .panel-mainview[panelid=identity-popup] > #identity-popup-mainView {
  /* Tiny hack to ensure the panel shrinks back to its original
     size after closing a subview that is bigger than the main view. */
  max-height: 0;
}

It seems that the "shrinking" still works with the max-height:0 removed...
Disregard that last line. Shrinking doesn't happen without the max-height.
It seems another trick is needed to get the stack to shrink when its contents shrink...
Whiteboard: [fxprivacy] → [fxprivacy] [campaign]
(In reply to Alfred Kayser from comment #18)
> Removing this "tiny-hack" does seem to resolve this issue:
> #identity-popup,
> .panel-viewstack[viewtype="main"]:not([transitioning]) >
> .panel-mainview[panelid=identity-popup] > #identity-popup-mainView {
>   /* Tiny hack to ensure the panel shrinks back to its original
>      size after closing a subview that is bigger than the main view. */
>   max-height: 0;
> }

Thanks, it does look like that fixes the reported problem!  I'll have to see if finding a new workaround for shrinking is easier or harder than converting it to css flexbox.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Iteration: --- → 42.2 - Jul 27
Bug 1184447 - Remove max-height on the #identity-popup-mainView to prevent the 'more information' button from being clipped;r=ttaubert
Attachment #8639600 - Flags: review?(ttaubert)
Tim, I couldn't find specifically why the selector `.panel-viewstack[viewtype="main"]:not([transitioning]) > .panel-mainview[panelid=identity-popup] > #identity-popup-mainView` was added to the list for max-height: 0 in Bug 1170759.  But removing it allows the 'more information' button to be fully clickable and preserves the shrinking behavior when testing on a page like: https://www.mozilla.org/en-US/ or https://people.mozilla.org/~tvyas/AllMixedTests.html.
Comment on attachment 8639600 [details]
MozReview Request: Bug 1184447 - Convert control center main-view to use CSS flexbox;r=ttaubert

https://reviewboard.mozilla.org/r/14217/#review12877

The problem is that the panel doesn't shrink correctly when we need it smaller: http://i.imgur.com/5v9Dbkv.png Sorry :(
Attachment #8639600 - Flags: review?(ttaubert)
It seems that the same layout problem hits bug 959567.
Iteration: 42.2 - Jul 27 → 42.3 - Aug 10
STR:

1) Open a new window.
2) Open a new tab.
3) Load mozilla.org
4) Open the control center.
5) Switch back to the about:home tab
6) Open the control center.
Attachment #8639600 - Attachment description: MozReview Request: Bug 1184447 - Remove max-height on the #identity-popup-mainView to prevent the 'more information' button from being clipped;r=ttaubert → MozReview Request: Bug 1184447 - Convert control center main-view to use CSS flexbox;r=ttaubert
Attachment #8639600 - Flags: review?(ttaubert)
Comment on attachment 8639600 [details]
MozReview Request: Bug 1184447 - Convert control center main-view to use CSS flexbox;r=ttaubert

Bug 1184447 - Convert control center main-view to use CSS flexbox;r=ttaubert

This works around the clipped 'more information' button that shows up when there is
wrapping inside one of the labels inside the main view.
I've submitted a patch to go along with roc's idea in Comment 17 about converting to CSS flexbox.  This seems to fix the 'more information' bug, although I think a fix in the layout engine would be ideal -  I could imagine some weird behavior with this workaround if some element had a more specific or important moz-box-flex display being set, or relied on some other xul styles that aren't covered in this conversion.

I thought it would be cool to introduce a new [flexbox=true] attribute we could add to any xul element that would more fully shim the remaining pack / align / flex attributes but it seemed out of scope here.  The plus side to doing something like that would be that we wouldn't need to remember to add new styles inside CSS if we started using them in the main view (like align="end"), but this is a pretty small element so keeping that up to date shouldn't be too hard.
Before giving up I dug into the failure some more with the small test case that Brian provided. It seems that nsFrame::GetMinSize() returns the wrong minimum size for the <label>, and thus its parent <vbox> and the grandparent <hbox>. After nsFrame::DoLayout() sees that the <label> grows to height=38px, GetMinSize() still returns a height=19px (for one line instead of two lines).

After the <label> grew it seems that the size doesn't get invalidated, and nsFrame::RefreshSizeCache() bails out early. So that might be one bug?

If I "fix" RefreshSizeCache() to assign metrics->mBlockMinSize.height=38px to the <label> then everything seems to get rendered properly. In RefreshSizeCache(), the LineIterator shows two lines, the second has a zero height though.

Not knowing a lot about all of this my two thoughts are: should RefreshSizeCache() be fixed to correctly compute mBlockMinSize.height? It seems weird that the code assigns the height of the highest line instead of accumulating all line heights?

And secondly, should the second line have a height > 0 somehow?
Flags: needinfo?(roc)
The fix described above does work with the actual control center too. The wrapping label gets a min-height of 30px instead of 15px and the "More Information" button behaves fine.
Actually I think you're right. I don't know why we're taking the max of the line heights instead of just using metrics->mBlockMinSize.height = desiredSize.Height() like we do for the non-lines case.

Making that change is a bit scary, because it could change a lot of XUL layouts, but it does seem reasonable.
Flags: needinfo?(roc)
Comment on attachment 8639600 [details]
MozReview Request: Bug 1184447 - Convert control center main-view to use CSS flexbox;r=ttaubert

This is a valid solution but I have a simpler suggestion, not totally sure if better.
Attachment #8639600 - Flags: review?(ttaubert) → feedback+
Here's my suggestion. I played around with the layout code and found some interesting things but in the end I just didn't understand what needs to be done. For someone with a tiny bit of layout knowledge that might maybe not too hard?

Anyway, I picked up your first patch again and as you already found out it's indeed max-height:0 that confuses XUL layout somehow.

As we need that only to properly shrink the panel when we update it we could just not apply it when the panel is opened, which is what the patch does. We almost never update the panel when it's open in a normal browser interaction, and with bug 1175702 we will completely stop doing that.

It seems to work fine for me but having a second pair of eyes would be great. It doesn't make the layout simpler or easier to understand but it seems the simplest (and maybe safest?) solution.

So... what do you think?
Attachment #8643662 - Flags: review?(bgrinstead)
Comment on attachment 8643662 [details] [diff] [review]
0001-Bug-1184447-Fix-XUL-layout-bug-with-wrapping-labels-.patch

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

This fixes the problem for me, nice
Attachment #8643662 - Flags: review?(bgrinstead) → review+
Attachment #8639600 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/162181854741
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Verified as fixed on:

FF 42
Build Id:20150811030206
OS: Win 7 x64, Ubuntu 12.04 x86, Mac Os X 10.10.4
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.