Closed Bug 1448555 Opened 2 years ago Closed 2 years ago

Browser window cannot be shrunk when the devtools is taken the largest width/height

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox61+ fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 + fixed

People

(Reporter: xidorn, Assigned: bgrins)

References

Details

Attachments

(3 files, 1 obsolete file)

Steps to reproduce:
1. have the browser window in normal state (not maximized)
2. open the devtools and have it attached to the bottom of page
3. size the devtools to take the maximum possible height
4. try resizing the browser window

Expected result:
the browser window should still be shrinkable.

Actual result:
the browser window is not shrinkable.


This is because devtools use <iframe>, and height attribute on <iframe> is its preferred size. And when sizing inside XUL, an element with no "flex" attribute set means its min intrinsic size is its preferred size.

So when the devtools takes the maximum size, the window would have a size constraint that prevents it from being shrunk.
Probably the most straightforward way to fix this is to set flex attribute on the <iframe> so that it doesn't stretch the window.

However, that would fail at least browser_toolbox_hosts_size.js for size not being correctly applied, for which I have no idea.
Comment on attachment 8962061 [details]
Bug 1448555 - Make devtools flexible so that it doesn't stretch the window.

https://reviewboard.mozilla.org/r/230892/#review236360


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/framework/toolbox-hosts.js:54
(Diff revision 1)
> +function shrinkElementBefore(container, frame, axis) {
> +  let element;
> +  for (let child = container.lastElementChild; child;
> +       child = child.previousElementSibling) {
> +    if (child.getClientRects().length > 0 &&
> +        parseInt(child.getAttribute("flex")) > 0) {

Error: Missing radix parameter. [eslint: radix]
FWIW we bumped into some sizing issues with the iframe during flexbox emulation. To support CSS flexbox we wanted to stop using the width and height attributes altogether (and use style.height / style.width instead). I had a patch that did that but Julian pointed out an issue with it (the <splitter> was only setting [height] attr) so we spun it out into a follow up bug (Bug 1434080): https://reviewboard.mozilla.org/r/216376/diff/1#index_header.
See Also: → 1434080
(In reply to Brian Grinstead [:bgrins] from comment #4)
> FWIW we bumped into some sizing issues with the iframe during flexbox
> emulation. To support CSS flexbox we wanted to stop using the width and
> height attributes altogether (and use style.height / style.width instead). I
> had a patch that did that but Julian pointed out an issue with it (the
> <splitter> was only setting [height] attr) so we spun it out into a follow
> up bug (Bug 1434080):
> https://reviewboard.mozilla.org/r/216376/diff/1#index_header.

Thanks for the information. Would that change fix the issue described in comment 0? Is there any estimation on when would that get fixed?

It seems the patch has been there for a while without much activity, so I tend to fix this first to unblock bug 1447056.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #6)
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > FWIW we bumped into some sizing issues with the iframe during flexbox
> > emulation. To support CSS flexbox we wanted to stop using the width and
> > height attributes altogether (and use style.height / style.width instead). I
> > had a patch that did that but Julian pointed out an issue with it (the
> > <splitter> was only setting [height] attr) so we spun it out into a follow
> > up bug (Bug 1434080):
> > https://reviewboard.mozilla.org/r/216376/diff/1#index_header.
> 
> Thanks for the information. Would that change fix the issue described in
> comment 0? Is there any estimation on when would that get fixed?
> 
> It seems the patch has been there for a while without much activity, so I
> tend to fix this first to unblock bug 1447056.

I'm not sure if it would fix the issue - would using style.height have the same behavior as the height attribute regarding preferred / intrinsic sizing? Either way, feel free to proceed here if it'll unblock other stuff.
Actually... it doesn't seem to have any difference. As far as the <iframe> still doesn't have flex, the height property would still be treated as preferred size and thus min intrinsic size.

So if that bug is just changing it to use style.height instead of height attribute, then probably nothing would change for this bug.
Attachment #8962061 - Flags: review?(jryans) → review?(bgrinstead)
It sounds like :bgrins has more context on this issue, so I redirected the review to him.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #0)
> Steps to reproduce:
> 1. have the browser window in normal state (not maximized)
> 2. open the devtools and have it attached to the bottom of page
> 3. size the devtools to take the maximum possible height
> 4. try resizing the browser window
> 
> Expected result:
> the browser window should still be shrinkable.
> 
> Actual result:
> the browser window is not shrinkable.
> 
> This is because devtools use <iframe>, and height attribute on <iframe> is
> its preferred size. And when sizing inside XUL, an element with no "flex"
> attribute set means its min intrinsic size is its preferred size.
> 
> So when the devtools takes the maximum size, the window would have a size
> constraint that prevents it from being shrunk.

I tried setting flex='0' (and -moz-box-flex: 0) on the iframe but I still see the same result. Is there something else that needs to be done to allow the devtools window to be shrunk below it's preferred size?
Flags: needinfo?(xidorn+moz)
(In reply to Brian Grinstead [:bgrins] from comment #10)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #0)
> > Steps to reproduce:
> > 1. have the browser window in normal state (not maximized)
> > 2. open the devtools and have it attached to the bottom of page
> > 3. size the devtools to take the maximum possible height
> > 4. try resizing the browser window
> > 
> > Expected result:
> > the browser window should still be shrinkable.
> > 
> > Actual result:
> > the browser window is not shrinkable.
> > 
> > This is because devtools use <iframe>, and height attribute on <iframe> is
> > its preferred size. And when sizing inside XUL, an element with no "flex"
> > attribute set means its min intrinsic size is its preferred size.
> > 
> > So when the devtools takes the maximum size, the window would have a size
> > constraint that prevents it from being shrunk.
> 
> I tried setting flex='0' (and -moz-box-flex: 0) on the iframe but I still
> see the same result. Is there something else that needs to be done to allow
> the devtools window to be shrunk below it's preferred size?

nm, I guess setting flex='0' is the same as omitting it: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/flex. I think I may have found a different solution to this issue though, similar to how we handle the split console in the toolbox. Will upload patches.
Flags: needinfo?(xidorn+moz)
Still waiting on try results but the browser_toolbox_hosts_size.js test passes with this, and the toolbox / window resizing generally feels right. Julian: could you confirm that resizing continues to work as you expect with the patch applied?
Attachment #8962061 - Attachment is obsolete: true
Attachment #8962061 - Flags: review?(bgrinstead)
Looks like Brian has a simpler solution for this.
Assignee: nobody → bgrinstead
Attachment #8962442 - Flags: review?(gijskruitbosch+bugs) → review?(dao+bmo)
Comment on attachment 8962442 [details]
Bug 1448555 - Part 1 - Allow the devtools toolbox to shrink when the window shrinks

https://reviewboard.mozilla.org/r/231286/#review237060

r=me, but I'd like Dão to sanity-check this too.
Attachment #8962442 - Flags: review+
Comment on attachment 8962443 [details]
Bug 1448555 - Part 2 - Increase flex from 1000 to 10000 for the devtools primary tool;

https://reviewboard.mozilla.org/r/231288/#review237066

::: toolkit/content/xul.css:1019
(Diff revision 1)
>    [flex="1"] { -moz-box-flex: 1; }
>    [flex="2"] { -moz-box-flex: 2; }
>    [flex="3"] { -moz-box-flex: 3; }
>    [flex="4"] { -moz-box-flex: 4; }
>    [flex="5"] { -moz-box-flex: 5; }
>    [flex="6"] { -moz-box-flex: 6; }
>    [flex="7"] { -moz-box-flex: 7; }
>    [flex="8"] { -moz-box-flex: 8; }
>    [flex="9"] { -moz-box-flex: 9; }
>    [flex="100"] { -moz-box-flex: 100; }
>    [flex="400"] { -moz-box-flex: 400; }
>    [flex="1000"] { -moz-box-flex: 1000; }
> +  [flex="10000"] { -moz-box-flex: 10000; }

Actually... I would have suggested you to implement this via attribute mapping mechanism rather than this kind of stuff, although the good side is wrapping stuff in `@supports -moz-bool-pref` is cheaper when the pref is not enabled (but much more expensive when it's enabled because attribute selector is relatively slow).

Also I would want to avoid using `attr()` as that could have performance issue as well depending on how would we track that...
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #17)
> Comment on attachment 8962443 [details]
> Bug 1448555 - Part 2 - Increase flex from 1000 to 10000 for the devtools
> primary tool;
> 
> https://reviewboard.mozilla.org/r/231288/#review237066
> 
> ::: toolkit/content/xul.css:1019
> (Diff revision 1)
> >    [flex="1"] { -moz-box-flex: 1; }
> >    [flex="2"] { -moz-box-flex: 2; }
> >    [flex="3"] { -moz-box-flex: 3; }
> >    [flex="4"] { -moz-box-flex: 4; }
> >    [flex="5"] { -moz-box-flex: 5; }
> >    [flex="6"] { -moz-box-flex: 6; }
> >    [flex="7"] { -moz-box-flex: 7; }
> >    [flex="8"] { -moz-box-flex: 8; }
> >    [flex="9"] { -moz-box-flex: 9; }
> >    [flex="100"] { -moz-box-flex: 100; }
> >    [flex="400"] { -moz-box-flex: 400; }
> >    [flex="1000"] { -moz-box-flex: 1000; }
> > +  [flex="10000"] { -moz-box-flex: 10000; }
> 
> Actually... I would have suggested you to implement this via attribute
> mapping mechanism rather than this kind of stuff, although the good side is
> wrapping stuff in `@supports -moz-bool-pref` is cheaper when the pref is not
> enabled (but much more expensive when it's enabled because attribute
> selector is relatively slow).
> 
> Also I would want to avoid using `attr()` as that could have performance
> issue as well depending on how would we track that...

Yeah, I don't expect we'd actually want to ship this block of code whenever we flip emulation mode on by default - it's just enough to get the primary UI to render properly. I figured we'd either rewrite frontend code to specify the flex properties in CSS, or handle it in the platform somehow. Would doing attribute mapping when in emulation mode be relatively easy / lightweight? I'm not actually sure how the current XUL flex handles these.
Flags: needinfo?(xidorn+moz)
Using classes / explicitly specifying the property in CSS would be best IMO. I'd rather not add more mapped attributes, they have a weird position in the cascade.
Comment on attachment 8962442 [details]
Bug 1448555 - Part 1 - Allow the devtools toolbox to shrink when the window shrinks

https://reviewboard.mozilla.org/r/231286/#review237110

Works fine with this patch, and looks good to me.
Attachment #8962442 - Flags: review?(jdescottes) → review+
Comment on attachment 8962443 [details]
Bug 1448555 - Part 2 - Increase flex from 1000 to 10000 for the devtools primary tool;

This looks good to me but :xidorn probably knows more about this than I do.
Attachment #8962443 - Flags: review?(jdescottes) → review?(xidorn+moz)
Comment on attachment 8962443 [details]
Bug 1448555 - Part 2 - Increase flex from 1000 to 10000 for the devtools primary tool;

https://reviewboard.mozilla.org/r/231288/#review237336

Sure, I don't have problem with this patch itself. Just some general suggestion for how this may be implemented.
Attachment #8962443 - Flags: review?(xidorn+moz) → review+
According to my (brief) read of some of the XUL code, I believe flex etc. attributes are just handled by the corresponding frames directly from DOM.

As emilio mentioned, specifying them with classes is probably the best approach at the end, but that would require you to know everything you would need.

Given what you already have there (flex / ordinal) it is probably easier to just define an attribute mapping so that the numbers in attributes can map to the CSS properties you need without manually adding rules for each different value. (The keyword ones can be done this way as well, but you would need to specify every single keyword so that's probably not very effective in terms of productivity.)
Flags: needinfo?(xidorn+moz)
Comment on attachment 8962443 [details]
Bug 1448555 - Part 2 - Increase flex from 1000 to 10000 for the devtools primary tool;

https://reviewboard.mozilla.org/r/231288/#review238110
Attachment #8962443 - Flags: review?(jdescottes) → review+
This is causing one animation inspector test to fail: https://treeherder.mozilla.org/#/jobs?repo=try&revision=091cbf4d3b32e56db7caa9bf5ccec015fe1084a8&selectedJob=171117542. I don't understand why this would happen, since the toolbox gets opened with the default height set. I can only reproduce locally in headless mode, which makes it harder to debug.
I've confirmed that the innerHeight of the animation inspector window is the same with or without the patches applied. But strangely, changing pref("devtools.toolbox.footer.height", 250) to pref("devtools.toolbox.footer.height", 251) causes the test to pass locally.
The failing assertion at https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/devtools/client/animationinspector/test/browser_animation_detail_easings.js#109 is basically just confirming that :hover class here gets applied: https://searchfox.org/mozilla-central/source/devtools/client/themes/animationinspector.css#705-707.

Makes me think there must be some issue with the call to `hintEl.scrollIntoView(false);` or `EventUtils.synthesizeMouseAtCenter(hintEl, {type: "mouseover"}, win);` but nothing is jumping out when logging out bounding client rects and scroll positions.
It's not like something is covering the element either - `hintEl === hintEl.ownerDocument.elementFromPoint(rect.left + (rect.width / 2), rect.top + (rect.height / 2)))` is true here with or without the patch: https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/devtools/client/animationinspector/test/browser_animation_detail_easings.js#103. And that's the same calculation that `EventUtils.synthesizeMouseAtCenter` (which is supposed to be triggering the hover state) does.
Patrick: I don't love this solution, but it's the best I could figure out - this is similar to what's done here https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/browser/components/extensions/test/browser/browser_ext_tabs_audio.js#167-169.  For whatever reason the svg element isn't getting put into a hover state even though its sizing hasn't changed.
Comment on attachment 8964057 [details]
Bug 1448555 - Part 0 - Manually set hover state on animation hint rather than relying on EventUtils;

https://reviewboard.mozilla.org/r/232854/#review238970

::: devtools/client/themes/animationinspector.css:706
(Diff revision 1)
>  .keyframes svg path.hint {
>    fill: none;
>  }
>  
> -.keyframes svg path.hint:hover {
> +.keyframes svg path.hint:hover,
> +.keyframes svg path.hint.hover {

Maybe add a short comment here saying we don't use the `hover` class in production code. Only in the test because of the flaky nature of mouseover simulation.
Attachment #8964057 - Flags: review?(pbrosset) → review+
Comment on attachment 8962442 [details]
Bug 1448555 - Part 1 - Allow the devtools toolbox to shrink when the window shrinks

https://reviewboard.mozilla.org/r/231286/#review239098
Attachment #8962442 - Flags: review?(dao+bmo) → review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f8ef62640179
Part 0 - Manually set hover state on animation hint rather than relying on EventUtils;r=pbro
https://hg.mozilla.org/integration/autoland/rev/64ba7c1bef89
Part 1 - Allow the devtools toolbox to shrink when the window shrinks;r=dao,Gijs,jdescottes
https://hg.mozilla.org/integration/autoland/rev/71ff44d934a0
Part 2 - Increase flex from 1000 to 10000 for the devtools primary tool;r=jdescottes,xidorn
I have reproduced this bug with Nightly 61.0a1 (2018-03-24) on Windows 10, 64 Bit!

This bug's fix is verified with latest Nightly!

Bulid ID   -  20180415220108
User Agent -  Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
QA Whiteboard: [bugday-20180411]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.