1417220 - stylo: "Confirm close" dialog box is truncated on Mac

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Chris Peterson [:cpeterson]]

Attachment: Confirm_close_screenshot.png

STR from Ovidiu:

1. Set the prefs to enable stylo-chrome: “layout.css.servo.chrome.enabled” = TRUE
2. Restart the browser
3. Make sure you have more than 2 tabs opened and then try to close the browser

Actual results:

The pop-up windows "Confirm close" is not displayed correctly. Please see the attached screenshot.

I used FF Nightly 59.0a1(2017-11-14) with Mac OS X 10.12. This is only reproducible on Mac OS, I tested on Windows 10 but I couldn't reproduce it.

P2 because we should fix this before shipping stylo-chrome but it doesn't block enabling stylo-chrome in Nightly.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

So, this is probably not Mac-specific. The dialog content is truncated on Mac, but I can also see that its vertical padding is shrunk on Windows. Supposedly they have the same root cause.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

For this bug, the xul file is chrome://global/content/commonDialog.xul

There is "padding-top: 8px; padding-bottom: 10px;" for the root <dialog> element from dialog.css. It seems that somehow this rule isn't working at the initial styling, but when I open inspector, the padding takes effect shortly.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

So, I think the issue is that, xbl binding style on the binding root is not applied to the element when the host element itself is a root element in its document.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch of the reftest

This is the reftest showing the underlying problem of this bug.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Does it work if you restyle the element again? Or it never works?

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

So I have figured out the reason here. It is because in nsCSSFrameConstructor::ConstructDocElementFrame, we don't clear the element style data when binding is changed like what we do in nsCSSFrameConstructor::AddFrameConstructionItemsInternal.

If I add the code (ClearServoDataFromSubtree / StyleNewSubtree / StyleNewChildren) into ConstructDocElementFrame, it would get the correct result. The only problem is that I'm not sure how to arrange the code.

emilio, since you put some comment around that part of code, do you have an idea about what is the best way to arrange the code? Should we abstract that somehow to make it clearer?

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Related code:
https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/layout/base/nsCSSFrameConstructor.cpp#2613-2630
https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/layout/base/nsCSSFrameConstructor.cpp#5926-5953

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1417220 - Force re-resolve style for doc element when binding requires so.

Review commit: https://reviewboard.mozilla.org/r/200076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200076/

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8928801 [details]
Bug 1417220 - Force re-resolve style for doc element when binding requires so.

https://reviewboard.mozilla.org/r/200076/#review205208

Nice fix, thanks!

r=me

::: layout/style/ServoStyleSet.h:185
(Diff revision 1)
>    ResolveStyleFor(dom::Element* aElement,
>                    ServoStyleContext* aParentContext,
>                    LazyComputeBehavior aMayCompute);
>  
> +  // Clear style data and resolve style for the given element and its
> +  // subtree for change to -moz-binding.

Mention that it returns the host element's style?

::: layout/style/ServoStyleSet.cpp:354
(Diff revision 1)
>    return ResolveServoStyle(aElement);
>  }
>  
> +already_AddRefed<ServoStyleContext>
> +ServoStyleSet::ReresolveStyleForBindings(Element* aElement)
> +{

Maybe we should assert somehow that the element's style has a binding? No big deal, but...

::: layout/style/ServoStyleSet.cpp:361
(Diff revision 1)
> +  ServoRestyleManager::ClearServoDataFromSubtree(aElement);
> +  StyleNewSubtree(aElement);
> +
> +  // Servo's should_traverse_children() in traversal.rs skips
> +  // styling descendants of elements with a -moz-binding the
> +  // first time. Thus call StyleNewChildren() again.

(The fact that we do this is stupid, and I had a patch removing it, but that's another story).

::: layout/style/ServoStyleSet.cpp:366
(Diff revision 1)
> +  // first time. Thus call StyleNewChildren() again.
> +  StyleNewChildren(aElement);
> +
> +  // Because of LazyComputeBehavior::Assert we never create a style
> +  // context here, so it's fine to pass a null parent.
> +  return ResolveStyleFor(aElement, nullptr, LazyComputeBehavior::Assert);

You can just call `ResolveServoStyle(aElement)`.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8928801 [details]
Bug 1417220 - Force re-resolve style for doc element when binding requires so.

https://reviewboard.mozilla.org/r/200076/#review205208

> Maybe we should assert somehow that the element's style has a binding? No big deal, but...

But I have no idea how can we trivially do that... We cannot get style data out from element directly in Gecko side... Maybe adding a FFI call to ask Servo check servo data is doable... But I guess it's not worth it.

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8928801 [details]
Bug 1417220 - Force re-resolve style for doc element when binding requires so.

https://reviewboard.mozilla.org/r/200076/#review205208

> But I have no idea how can we trivially do that... We cannot get style data out from element directly in Gecko side... Maybe adding a FFI call to ask Servo check servo data is doable... But I guess it's not worth it.

Well, you're getting the style of the element, and the caller has it, too. The binding is in `StyleDisplay()->mBinding`. But you're right it's a bit of churn, your call.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8928801 [details]
Bug 1417220 - Force re-resolve style for doc element when binding requires so.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200076/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/623f97519d81
Force re-resolve style for doc element when binding requires so. r=emilio

Comment 14

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/623f97519d81

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

The patch definitely fixed the issue I observed on Windows, but I'm not completely sure that it fixes the macOS issue, as there is a chance that they are different issues.

cpeterson, when the patch gets into Nightly, could you verify that?

Comment 16

[Chris Peterson [:cpeterson]]

(In reply to Xidorn Quan [:xidorn] UTC-8 (less responsive Nov 5 ~ Dec 16) from comment #15)
> The patch definitely fixed the issue I observed on Windows, but I'm not
> completely sure that it fixes the macOS issue, as there is a chance that
> they are different issues.
>
> cpeterson, when the patch gets into Nightly, could you verify that?

Yes, I verified that this bug is fixed on macOS in Nightly 59.0a1 build 2017-11-18.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

https://hg.mozilla.org/projects/oak/rev/623f97519d814fcc6554deeeff706fe15d176f9e
Bug 1417220 - Force re-resolve style for doc element when binding requires so. r=emilio