Closed Bug 1229278 Opened 4 years ago Closed 4 years ago

Dynamic changes to text-emphasis may not take effect

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: xidorn, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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.)
(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.)
(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.
(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.
(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.)
I believe at least text-shadow does UpdateSubtreeOverflow. text-emphasis should just follow the same way I suppose.
Bug 1229278 - Fix dynamic changes to text-emphasis-style.
Attachment #8694170 - Flags: review?(dbaron)
Comment on attachment 8694170 [details]
MozReview Request: Bug 1229278 - Fix dynamic changes to text-emphasis-style.

https://reviewboard.mozilla.org/r/26681/#review24205
Attachment #8694170 - Flags: review?(dbaron) → review+
Blocks: 1229609
https://hg.mozilla.org/mozilla-central/rev/67aea49920bc
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.