Closed Bug 1359834 Opened 7 years ago Closed 7 years ago

Dynamic changes to shape-outside do not correctly trigger reflow

Categories

(Core :: Layout: Floats, defect)

Product:
Core
Component:
Layout: Floats
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: pbro, Assigned: bradwerth)

References

Details

Attachments

(5 files, 1 obsolete file)

shape-outside-test.html
1.01 KB, text/html
Details
Bug 1359834 Part 3: Force changes to shape-outside to trigger reflow and overflow recalculation.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1359834 Part 4: Add a reftest to ensure dynamic changes to shape-outside trigger reflow.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1359834 Part 1: Change nsStyleDisplay::CalcDifference to early exit when hinting nsChangeHint_ReconstructFrame.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1359834 Part 2: Define a new nsChangeHint_ReflowHintsForFloatAreaChange hint and use it for float changes.
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
basic shapes for shape-outside landed with bug 1326409, so I was testing it a little bit and investigating how we would go about doing something in DevTools to help people debug shapes.

While doing so, I stumbled upon a problem for which I didn't see a bug for, so I'm filing one now.

2 different sets of steps to reproduce:

- Open https://mdn.mozillademos.org/fr/docs/Web/CSS/shape-outside$samples/Exemples?revision=1224039
- Open the DevTools inspector
- Select the .left element in the inspector
- In the CSS Rules sidebar panel, try to change the value of the shape-outside property to anything else, like circle()

==> Nothing changes in the page, the new value of the shape-outside is not taken into account.

This, in itself, got me thinking that this was a DevTools bug, but then I tried this other test case:

- Open the attached test case
- Click the pink dot several times

==> Nothing changes in the page either.
Clicking toggles an inline style on the pink element: shape-outside: circle()
It seems that the shape-outside value does not get applied on the element.
So I think this isn't an inspector bug.
If you open the inspector and then click, you should see the value of the inline style getting applied correctly in the inspector, but no changes to the layout of the page.

It looks like dynamic changes to this property via JS just don't work at the moment.

I'm not seeing anything in bug 1353631 or bug 1098939 about this particular problem.
I'm working on the CSS shapes highlighter in DevTools (bug 1242029 and related) and I've run into the same issue.

I have discovered, however, that if you change the element's width or height, the change to the shape-outside property becomes visible. If the element does not have a parent element besides <body>, resizing the window works as well. 

This can be reproduced in either of the above test cases by changing the shape-outside property to circle(), then changing the width of the element.

So it appears that changing the shape-outside property does not trigger a reflow of the text. It's not until something else does that the changes to shape-outside become visible.
Hi Brad, as Mike said in comment 1, we're working on a tool for css shapes, and this bug is forcing us to write some work around code that we'd like to get rid of.
Is this something you could possibly look into? Or find someone else?
Thanks!
Flags: needinfo?(bwerth)
Assignee: nobody → bwerth
Flags: needinfo?(bwerth)
Comment 1 is correct; this is a layout issue and not a devtools issue. Proposed patches should produce correct layout behavior and devtools should then act as expected.
Summary: The value of shape-outside can't be changed dynamically either by JS or via DevTools → Dynamic changes to shape-outside do not correctly trigger reflow
Attachment #8885874 - Flags: review?(dholbert)
Attachment #8885875 - Flags: review?(dholbert)
Comment on attachment 8885875 [details]
Bug 1359834 Part 4: Add a reftest to ensure dynamic changes to shape-outside trigger reflow.

https://reviewboard.mozilla.org/r/156642/#review162138

::: layout/reftests/css-shape/dynamic-shape-outside-ref.html:4
(Diff revision 1)
> +<!DOCTYPE html>
> +<html>
> +<head>
> +<!--<script>SpecialPowers.setBoolPref("layout.css.shape-outside.enabled", true);</script>-->

No need for this commented-out script -- let's get rid of it.

::: layout/reftests/css-shape/dynamic-shape-outside.html:4
(Diff revision 1)
> +<!DOCTYPE html>
> +<html>
> +<head>
> +<!--<script>SpecialPowers.setBoolPref("layout.css.shape-outside.enabled", true);</script>-->

As above, let's get rid of this line.

::: layout/reftests/css-shape/dynamic-shape-outside.html:7
(Diff revision 1)
> +<html>
> +<head>
> +<!--<script>SpecialPowers.setBoolPref("layout.css.shape-outside.enabled", true);</script>-->
> +<meta http-equiv="content-type" content="text/html; charset=UTF-8">
> +<title>Dynamic change to shape-outside</title>
> +<link rel="match" href="dynamic-shape-outside-ref.html">

Probably get rid of this <link rel="match"> -- it would only be used if we were upstreaming this test to the CSSWG, but I don't think we can, since it'll depend on MozReftestInvalidate per a comment below. (and the CSSWG reftest harness doesn't have an equivalent for MozReftestInvalidate)

::: layout/reftests/css-shape/dynamic-shape-outside.html:29
(Diff revision 1)
> +<script>
> +let circle = document.querySelector(".circle");
> +circle.style.shapeOutside = "circle()";
> +</script>

This dynamic change needs to be triggered by MozReftestInvalidate, in order to effectively test this bug.  (And then you need to use "reftest-wait" as well.)

Without that change, the script might get evaluated *before* we've ever done our first reflow, which would then mean the test would render with this "shape-outside" tweak up-front (in the initial reflow) and wouldn't be exercising dynamic updates at all.

::: layout/reftests/css-shape/reftest.list:1
(Diff revision 1)
> +pref(layout.css.shape-outside.enabled,true) == dynamic-shape-outside.html dynamic-shape-outside-ref.html

Please add a "-1" suffix to the filenames here:
  dynamic-shape-outside-1.html
  dynamic-shape-outside-1-ref.html
 
It's likely we'll want to add more tests in the future which exercise dynamic changes to shape-outside, so we'll want them to be named dynamic-shape-outside-1, 2, 3, etc.

::: layout/reftests/reftest.list:109
(Diff revision 1)
> +# css shape
> +include css-shape/reftest.list

Let's call this "css shapes" (plural), in both the comment and the directory name, since that's the name used in the spec itself (and its URL, https://www.w3.org/TR/css-shapes-1/ ) and presumably this directory is supposed to correspond to that spec.
Attachment #8885875 - Flags: review?(dholbert) → review-
Comment on attachment 8885874 [details]
Bug 1359834 Part 3: Force changes to shape-outside to trigger reflow and overflow recalculation.

https://reviewboard.mozilla.org/r/156640/#review162152

r=me on part 1 with nits:

::: layout/style/nsStyleStruct.cpp:3529
(Diff revision 1)
> -  if (mFloat != aNewData.mFloat) {
> -    // Changing which side we float on doesn't affect descendants directly
> +  if ((mFloat != aNewData.mFloat) ||
> +      (!mShapeOutside.DefinitelyEquals(aNewData.mShapeOutside))) {

Drop the inner parens around each of the two conditions here -- they're superflous (they aren't actually grouping anything).

::: layout/style/nsStyleStruct.cpp:3531
(Diff revision 1)
> +    // Changing which side we float on doesn't affect descendants directly.
> +    // Shape outside behaves very similarly.

Let's make this comment a bit less hand-wavy. How about:
    // Our descendants aren't impacted when our float area changes (e.g. if we
    // change which side we float to, or change 'shape-outside').  But our
    // ancestors/siblings are potentially impacted, so we need to send
    // the non-descendant reflow hints.
Attachment #8885874 - Flags: review?(dholbert) → review+
Comment on attachment 8885875 [details]
Bug 1359834 Part 4: Add a reftest to ensure dynamic changes to shape-outside trigger reflow.

https://reviewboard.mozilla.org/r/156642/#review162226

r=me with nit addressed

(And if you haven't already, please double-check that "./mach reftest layout/reftests/css-shapes" fails without Part 1, and passes with it. :))

::: layout/reftests/css-shapes/reftest.list:1
(Diff revision 2)
> +pref(layout.css.shape-outside.enabled,true) == dynamic-shape-outside-1.html dynamic-shape-outside-ref-1.html

Typo here -- "ref-1" should instead be "1-ref".

(It looks like you've got it correct in the actual filename -- it's only wrong in this reftest.list manifest.)
Attachment #8885875 - Flags: review?(dholbert) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ed0d72af39b8
Part 1: Force changes to shape-outside to trigger float reflow, similar to other float changes. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/074972bd94a6
Part 2: Add a reftest to ensure dynamic changes to shape-outside trigger reflow. r=dholbert
Backed out for failing mochitest test_inherit_computation.html:

https://hg.mozilla.org/integration/autoland/rev/c6604ffb107f06b32ad88bba665fb03eace5292c
https://hg.mozilla.org/integration/autoland/rev/2cc61cf5df02ef1f3ed7e20ae9b1c7b5ff167fa2

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=074972bd94a6cd7bfa7060809c1ad93acf0305a4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=114254414&repo=autoland

[task 2017-07-14T07:38:43.577705Z] 07:38:43     INFO - TEST-PASS | layout/style/test/test_inherit_computation.html | inherit should cause inheritance of initial value for 'shape-outside' 
[task 2017-07-14T07:38:43.579555Z] 07:38:43     INFO - Buffered messages finished
[task 2017-07-14T07:38:43.581527Z] 07:38:43     INFO - TEST-UNEXPECTED-FAIL | layout/style/test/test_inherit_computation.html | inherit should cause inheritance of initial value for 'shape-outside' - got "url(\"#my-shape-outside\")", expected "none"
[task 2017-07-14T07:38:43.587912Z] 07:38:43     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:312:5
[task 2017-07-14T07:38:43.591933Z] 07:38:43     INFO -     test_property/<@layout/style/test/test_inherit_computation.html:136:9
[task 2017-07-14T07:38:43.595950Z] 07:38:43     INFO -     test_property@layout/style/test/test_inherit_computation.html:56:3
[task 2017-07-14T07:38:43.597763Z] 07:38:43     INFO -     @layout/style/test/test_inherit_computation.html:173:3
Flags: needinfo?(bwerth)
Comment on attachment 8885874 [details]
Bug 1359834 Part 3: Force changes to shape-outside to trigger reflow and overflow recalculation.

https://reviewboard.mozilla.org/r/156640/#review162646

I'm downgrading part 1 to "r-" since it bounced, so that brad can re-request review when needed.

(Per IRC, I'm pretty sure that the proximal cause for the mochitest failure is that we're returning nsChangeHint(0) when shape-outside differs and "float" is none. We need to return nsChangeHint_NeutralChange instead, to get the style system to recognize that it actually needs to update itself to produce the correct computed style.)
Attachment #8885874 - Flags: review+ → review-
(And I think nsChangeHint_ReconstructFrame - the change hint in the patch that landed+bounced - is more severe than we need, given that we can produce the correct layout by resizing the window horizontally.)
Comment on attachment 8885874 [details]
Bug 1359834 Part 3: Force changes to shape-outside to trigger reflow and overflow recalculation.

https://reviewboard.mozilla.org/r/156640/#review162658

::: layout/style/nsStyleStruct.cpp:3496
(Diff revisions 4 - 5)
> -    hint |= nsChangeHint_ReconstructFrame;
> +    return nsChangeHint_ReconstructFrame;
> +  }
> +
> +  if ((mAppearance == NS_THEME_TEXTFIELD &&
> +       aNewData.mAppearance != NS_THEME_TEXTFIELD) ||
> +      (mAppearance != NS_THEME_TEXTFIELD &&
> +       aNewData.mAppearance == NS_THEME_TEXTFIELD)) {
> +    // This is for <input type=number> where we allow authors to specify a
> +    // |-moz-appearance:textfield| to get a control without a spinner. (The
> +    // spinner is present for |-moz-appearance:number-input| but also other
> +    // values such as 'none'.) We need to reframe since we want to use
> +    // nsTextControlFrame instead of nsNumberControlFrame if the author
> +    // specifies 'textfield'.
> +    return nsChangeHint_ReconstructFrame;
>    }

Could you separate these non-shape-outside related changes into their own commit?

::: layout/style/nsStyleStruct.cpp:3529
(Diff revisions 4 - 5)
>    if (mFloat != aNewData.mFloat) {
>      // Our descendants aren't impacted when our float area only changes
>      // placement but not size/shape. (e.g. if we change which side we float to).
>      // But our ancestors/siblings are potentially impacted, so we need to send
>      // the non-descendant reflow hints.
> -    hint |= nsChangeHint_AllReflowHints &
> +    hint |= nsChangeHint_ReflowHintsForISizeChange;

I'd rather we not make this simplification, I think... It's better to keep this using the longhand form here, IMO.

"float" changes are not qualitatively the same as "isize changes".  As it happens, we send the same set of changehints right now, but for different reasons -- and it's possible we'll add a new bit to nsChangeHint_ReflowHintsForISizeChange down the line which we do *not* want for "float" here.

(nsChangeHint_ReflowHintsForISizeChange is a relatively new mask, and it's intended to be used for scenarios where the isize itself really did change.)

::: layout/style/nsStyleStruct.cpp:3539
(Diff revisions 4 - 5)
> -    // If we are floating, and our shape-outside property changes, it requires
> -    // the entire frame to be reconstructed.
> -    hint |= nsChangeHint_ReconstructFrame;
> +      // If we are floating, and our shape-outside property changes, our
> +      // descendants are not impacted, but our ancestor and siblings are.
> +      // This is similar to a float-only change, but since the ISize of the
> +      // float area changes arbitrarily along its block axis, more is required
> +      // to get the siblings to adjust properly. Repaint is sufficient, but may
> +      // be too heavyweight.
> +      hint |= nsChangeHint_ReflowHintsForISizeChange |
> +              nsChangeHint_RepaintFrame;

I'd rather not use ReflowHintsForISizeChange here, too, for the same reasons as noted above for "float". (Though if you'd like, it might make sense to create a local variable which we define explicitly, called e.g. "reflowHintsForFloatAreaChange" -- set to AllReflowHints without ClearDescendantIntrinsics and NeedDirtyReflow -- and use that in both the float-difference and shape-outside-difference scenario.  This would let you keep this relatively short, and would be nicely self-documenting.)


Also: nsChangeHint_RepaintFrame doesn't make sense to me here. Its documentation is:
  // Invalidates all descendant frames (including following
  // placeholders to out-of-flow frames).

https://dxr.mozilla.org/mozilla-central/rev/7d92f47379da55adf4da6d0b16398f5c629cf949/layout/base/nsChangeHint.h#23-25

When an element's "shape-outside" changes, I don't see why we'd need to invalidate its descendants... We *might* need to invalidate some of its siblings, but its _descendants_ shouldn't need a repaint...

If the existing reflow-related change hints aren't sufficient, though, then we miiiight want nsChangeHint_SchedulePaint (which is kind of confusingly-named -- it triggers displaylist-based invalidation)...  I'm not immediately clear on why the reflow hints by themselves wouldn't already be sufficient to get the correct invalidation though.
Attachment #8885874 - Flags: review?(dholbert) → review-
Comment on attachment 8886732 [details]
Bug 1359834 Part 2: Define a new nsChangeHint_ReflowHintsForFloatAreaChange hint and use it for float changes.

https://reviewboard.mozilla.org/r/157530/#review162662

::: layout/base/nsChangeHint.h:430
(Diff revision 1)
> +// * For float changes, we need all reflow hints, but not the ones that
> +// apply to descendants.

s/float changes/changes to the float area of an already-floated element/

(Just to be extra clear that this isn't about "float:none" <--> "float:left" changes, or changes to the float-related "clear" property, for example.)

::: layout/style/nsStyleStruct.cpp:3530
(Diff revision 1)
> -    // Changing which side we float on doesn't affect descendants directly
> -    hint |= nsChangeHint_AllReflowHints &
> -            ~(nsChangeHint_ClearDescendantIntrinsics |
> -              nsChangeHint_NeedDirtyReflow);
> +    // Our descendants aren't impacted when our float area only changes
> +    // placement but not size/shape. (e.g. if we change which side we float to).
> +    // But our ancestors/siblings are potentially impacted, so we need to send
> +    // the non-descendant reflow hints.

This comment doesn't doesn't really make sense here anymore, now that the specific bit-masking-for-descendant-hints is happening somewhere else.  (That's what it was documenting, I think.)

Some subset version of this comment might want to migrate into your new stuff in nsChangeHint.h, perhaps -- and the only thing you'll wanna say here is something like:
 
  // Changing which side we're floating on (float:none was handled above)
Comment on attachment 8886731 [details]
Bug 1359834 Part 1: Change nsStyleDisplay::CalcDifference to early exit when hinting nsChangeHint_ReconstructFrame.

https://reviewboard.mozilla.org/r/157528/#review162664

r=me, assuming this passes tests (which I imagine it should), though please update the commit message as noted:

::: commit-message-f89c9:1
(Diff revision 1)
> +Bug 1359834 Part 1: Change nsStyleDisplay::CalcDifference to early exit when hinting nsChangeHint_ReconstructFrame.

You should add a second line here explaining the "why" of this change. Maybe something like:

If we'll be reconstructing frames for the subtree in question, then we'll also be reflowing & repainting that whole subtree.  So all of this function's other changehints become unnecessary & redundant.
Attachment #8886731 - Flags: review+
Comment on attachment 8885874 [details]
Bug 1359834 Part 3: Force changes to shape-outside to trigger reflow and overflow recalculation.

https://reviewboard.mozilla.org/r/156640/#review162666

::: commit-message-bba83:1
(Diff revision 6)
> +Bug 1359834 Part 3: Force changes to shape-outside to trigger reflow and repaint.

s/repaint/DLBI/ to be more precise about what sort of paint you're talking about.

::: layout/style/nsStyleStruct.cpp:3543
(Diff revision 6)
> +      // to get the siblings to adjust properly. Scheduling repaint is
> +      // sufficient, but may be too heavyweight.

If you're sure that the test breaks without SchedulePaint here, then I'm fine with it, though let's document the uncertainty with an // XXX comment, to call out that uncertainty a bit more clearly.

(I'm still not clear on *why* it's needed, but it's cheap enough [particularly given that this feature's still disabled] that I don't want to rabbithole too much on that.)
Attachment #8885874 - Flags: review?(dholbert) → review+
Comment on attachment 8886731 [details]
Bug 1359834 Part 1: Change nsStyleDisplay::CalcDifference to early exit when hinting nsChangeHint_ReconstructFrame.

https://reviewboard.mozilla.org/r/157528/#review163106

r=me with the commit message split into several lines, as noted below:

::: commit-message-f89c9:1
(Diff revision 2)
> +Bug 1359834 Part 1: Change nsStyleDisplay::CalcDifference to early exit when hinting nsChangeHint_ReconstructFrame. If we'll be reconstructing frames for the subtree in question, then we'll also be reflowing & repainting that whole subtree. So all of this function's other changehints become unnecessary and redundant.

Everything after the first sentence here should be on a separate line (separated by a blank line if you like, doesn't matter) -- and then all of that after-the-first-line information should be wrapped to 80 characters like any other code would.

(The first line of a commit message has special significance, because it's shown as the "short summary" and is displayed in e.g. the pushlog. All the remaining lines can provide extra background.)
Attachment #8886731 - Flags: review+
Comment on attachment 8886732 [details]
Bug 1359834 Part 2: Define a new nsChangeHint_ReflowHintsForFloatAreaChange hint and use it for float changes.

https://reviewboard.mozilla.org/r/157530/#review163112

r=me
Attachment #8886732 - Flags: review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd2693278ba1
Part 1: Change nsStyleDisplay::CalcDifference to early exit when hinting nsChangeHint_ReconstructFrame. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/12e9e2993964
Part 2: Define a new nsChangeHint_ReflowHintsForFloatAreaChange hint and use it for float changes. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/45b3e5e97dc7
Part 3: Force changes to shape-outside to trigger reflow and overflow recalculation. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/b90b259fdfa8
Part 4: Add a reftest to ensure dynamic changes to shape-outside trigger reflow. r=dholbert
Flags: needinfo?(bwerth)
This is the overflow that bug 1359834 was trying to update, instead it used
CSSOverflowChange, which is for 'overflow' property changes, and that causes
reframes except when on the <body> or <html> elements.

This is covered by existing tests.
Attachment #9013947 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: