718157 - Inconsistent layout with percentage text-indent

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Jesse Ruderman]

Attachment: testcase

1. Load the testcase
2. Click the first button, "Widen the outer box"
3. Click the second button, "Force re-layout"

Result: The 'X' moves at step 3.

Expected: I don't know what should happen at step 2, but step 3 should have no effect.

(Why is the text-indent working as a percentage of the outer box's width, rather than a percentage of the inner box's width?)

Comment 1

[Boris Zbarsky [:bzbarsky]]

> Why is the text-indent working as a percentage of the outer box's width

Because the spec says so.  http://www.w3.org/TR/CSS21/text.html#indentation-prop says:

  Percentages: refer to width of containing block 

Amusingly enough, Opera has this bug too.

I wonder whether we're managing to optimize away the inner block's reflow entirely or something....

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Not sure why bug 45631's patch didn't fix this.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Er, probably because of what bz said at the end of comment 1.)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, in particular, nsBlockFrame::Reflow does:

  if (!(GetStateBits() & NS_FRAME_IS_DIRTY) && reflowState->IsIResize()) {
    PrepareResizeReflow(state);
  }

so we don't even call PrepareResizeReflow if only the parent resized.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Fix dynamic changes of percentage text-indent when the parent's size changes and the element's size does not

This code needs to move out of PrepareResizeReflow() (and things that
trigger code in it) because PrepareResizeReflow is conditioned on the
block itself resizing.

The reftest is based on a test by Jesse Ruderman <[e-mail]>.

The reftest fails without the patch and passes with the patch.

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8562537 [details] [diff] [review]
Fix dynamic changes of percentage text-indent when the parent's size changes and the element's size does not

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

Patch looks fine, given the current spec, although in this case I think the spec is bad. But that's orthogonal to fixing the dynamic-change bug.

::: layout/generic/nsBlockFrame.cpp
@@ +1148,5 @@
> +      reflowState->mCBReflowState->IsIResize() &&
> +      reflowState->mStyleText->mTextIndent.HasPercent() &&
> +      !mLines.empty()) {
> +    mLines.front()->MarkDirty();
> +  }

You could put both this and the preceding statement into a shared NS_FRAME_IS_DIRTY condition, to avoid the repeated testing of that flag. Though I expect the difference is insignificant, if you prefer the readability of this version.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think I do prefer it as two separate if statements.

https://hg.mozilla.org/integration/mozilla-inbound/rev/11e7ca322c9d

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/11e7ca322c9d