1229278 - Dynamic changes to text-emphasis may not take effect

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

If script changes text-emphasis-style or text-emphasis-position dynamically, the new value may not take effect until next reflow.

The emphasis marks info is currently updated in nsTextFrame::UnionAdditionalOverflow, which is called by nsTextFrame::ReflowText and nsTextFrame::RecomputeOverflow. And we return nsChangeHint_UpdateOverflow for those changes which I supposed should triggers RecomputeOverflow somehow. So it isn't quite clear what's the reason of this issue. Probably I misunderstood the meaning of UpdateOverflow.

Dynamic changes to text-emphasis-position will be fixed in bug 1225018 because changing to that property would always triggers reflow since that bug. But text-emphasis-style won't be fixed there.

Comment 1

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

RecomputeOverflow is a specific thing that happens during inline reflow, because we can't determine the overflow of a text frame until the entire line has been laid out.

UpdateOverflow is a general mechanism that works on all frames, that is designed to be used for certain style changes that change overflow but don't require reflow.

nsTextFrame has a RecomputeOverflow method and an UpdateOverflow method.  (We should probably add some comments pointing from one to the other, explaining this difference.)

Comment 2

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

(Also, since nsChangeHint_UpdateOverflow is a change hint that doesn't imply the handling of the change for all descendants, we can't use the UpdateOverflow hint for inherited properties, which means that we should never actually call UpdateOverflow on text frames... at least not from style change handling.)

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #2)
> (Also, since nsChangeHint_UpdateOverflow is a change hint that doesn't imply
> the handling of the change for all descendants, we can't use the
> UpdateOverflow hint for inherited properties, which means that we should
> never actually call UpdateOverflow on text frames... at least not from style
> change handling.)

But text frames should inherit those properties from their ancestors, and get CalcDifference called on style struct of themselves, no? There is a UpdateSubtreeOverflow hint, but I suppose that is only for things like text decorarion which isn't inherited but does affect descendents.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #1)
> RecomputeOverflow is a specific thing that happens during inline reflow,
> because we can't determine the overflow of a text frame until the entire
> line has been laid out.
> 
> UpdateOverflow is a general mechanism that works on all frames, that is
> designed to be used for certain style changes that change overflow but don't
> require reflow.
> 
> nsTextFrame has a RecomputeOverflow method and an UpdateOverflow method. 
> (We should probably add some comments pointing from one to the other,
> explaining this difference.)

Yeah, I think I checked nsTextFrame::UpdateOverflow method, and confirmed it does call RecomputeOverflow. I just checked that function again, now I guess this issue is because that function returns earlier because no decoration block is found.

Comment 5

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

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #3)
> But text frames should inherit those properties from their ancestors, and
> get CalcDifference called on style struct of themselves, no? There is a
> UpdateSubtreeOverflow hint, but I suppose that is only for things like text
> decorarion which isn't inherited but does affect descendents.

Right, the UpdateSubtreeOverflow hint would lead to calling UpdateOverflow on nsTextFrames.

(We shouldn't use either hint for an inherited property, and I think we have assertions to check that, and text frames can't inherit non-inherited properties since they don't have any specified style.)

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

I believe at least text-shadow does UpdateSubtreeOverflow. text-emphasis should just follow the same way I suppose.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1229278 - Fix dynamic changes to text-emphasis-style.

Bug 1229278 - Fix dynamic changes to text-emphasis-style.

Comment 8

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

Comment on attachment 8694170 [details]
MozReview Request: Bug 1229278 - Fix dynamic changes to text-emphasis-style.

https://reviewboard.mozilla.org/r/26681/#review24205

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ab2f68aa0e2bbae1871e1a6f421ba0e8b8a076c
Bug 1229278 - Fix dynamic changes to text-emphasis-style. r=dbaron

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/835de4b7bcb2b9016ef425db25878929c28a9448
Backed out 8 changesets (bug 1225018, bug 1229278, bug 1224013) for reftest failures on CLOSED TREE

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67aea49920bc30e931b0f27fbdffc88218d32265
Bug 1229278 - Fix dynamic changes to text-emphasis-style. r=dbaron

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/67aea49920bc