Closed Bug 1417220 Opened 2 years ago Closed 2 years ago

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

Categories

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

57 Branch
Unspecified
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- wontfix
firefox59 --- verified

People

(Reporter: cpeterson, Assigned: xidorn)

References

Details

Attachments

(3 files)

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.
Summary: stylo-chrome: "Confirm close" dialog box is truncated on Mac → stylo: "Confirm close" dialog box is truncated on Mac
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.
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.
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.
This is the reftest showing the underlying problem of this bug.
Does it work if you restyle the element again? Or it never works?
Flags: needinfo?(xidorn+moz)
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?
Flags: needinfo?(xidorn+moz)
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)`.
Attachment #8928801 - Flags: review?(emilio) → review+
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 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.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/623f97519d81
Force re-resolve style for doc element when binding requires so. r=emilio
https://hg.mozilla.org/mozilla-central/rev/623f97519d81
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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?
Flags: needinfo?(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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(cpeterson)
https://hg.mozilla.org/projects/oak/rev/623f97519d814fcc6554deeeff706fe15d176f9e
Bug 1417220 - Force re-resolve style for doc element when binding requires so. r=emilio
Duplicate of this bug: 1417993
You need to log in before you can comment on or make changes to this bug.