Closed Bug 1609663 Opened 3 years ago Closed 3 years ago

SVG elements won't display text if a text element in a pattern is removed and a new text element is appended

Categories

(Core :: SVG, defect, P3)

72 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox-esr68 --- unaffected
firefox76 --- wontfix
firefox77 --- wontfix
firefox78 --- fixed

People

(Reporter: milchmann, Assigned: heycam)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Attached file svg_test.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/79.0.3945.117 Safari/537.36

Steps to reproduce:

  • SVG element with a pattern
  • Pattern has <text> element with text
  • Pattern is used in the fill of a <rect> element
  • The <text> element is removed from the pattern
  • A new <text> element is added to the pattern

A small sample is attached as file.

Actual results:

The SVG initially shows the text when loading the page. After the <text> element in the pattern is removed, and a new one is appended, no new text is shown anymore.

Expected results:

After a new <text> element is added to the pattern, the text content should be displayed.

This still worked in Firefox 70 and also works in Chrome.

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → SVG
Product: Firefox → Core
Priority: -- → P3
Regressed by: 1594949
Has Regression Range: --- → yes

Hi Boris, it looks like this is a regression from a commit that you landed (see comment above). Can you take a look when you get a chance?

Flags: needinfo?(boris.chiou)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → boris.chiou
Flags: needinfo?(boris.chiou)

Thanks for running mozregression for this. I will take a look at this sooner or later. :)

OK. Looks like the order of the initialization of the SVGTextFrame and nsFrame has some isseus:

  1. Call SVGTextFrame::Init(), which calls its parents' Init()s before setting NS_FRAME_IS_SVG_TEXT bit.
  2. Call nsFrame::Init(), and then this function calls nsFrame::DidSetComputedStyle(nullptr)
  3. nsFrame::DidSetComputedStyle() calls MaybeScheduleReflowSVGNonDisplayText()
  4. MaybeScheduleReflowSVGNonDisplayText early returns because nsSVGUtils::IsInSVGTextSubtree(aFrame) is false. The reason is that we haven't set the svg text frame bit yet. We set the svg text frame bit right after parents' Init()s.

So we don't call ScheduleReflowSVGNonDisplayText in this case. This may be an existing issue.

Another issue is: its anonBlock still has NS_FRAME_FIRST_REFLOW bit, so we still don't reflow SVG non-display text.

So right now we have to do 2 things I think:

  1. We have to call AddStateBits(... NS_FRAME_SVG_LAYOUT | NS_FRAME_IS_SVG_TEXT); before calling nsSVGDisplayContainerFrame::Init(); to make sure we set the correct bits before doing other things.
  2. Need to handle the case of removing/reinserting text frame in MaybeScheduleReflowSVGNonDisplayText() properly, especially when checking NS_FRAME_FIRST_REFLOW.
Assignee: boris.chiou → cam

Hi,
Since this problem is kind of critical for us and we don't know how bugs are handled at Mozilla, I would like to ask if there is an ETA for a fix, or if the bug could be indefinitely on hold.
Thank you very much.

Flags: needinfo?(cam)
Flags: needinfo?(boris.chiou)

I'd like to wait for Cameron's feedback first because he intends to fix this some weeks ago.

Thanks for starting the investigation, Boris.

(In reply to Boris Chiou [:boris] from comment #7)

So right now we have to do 2 things I think:

  1. We have to call AddStateBits(... NS_FRAME_SVG_LAYOUT | NS_FRAME_IS_SVG_TEXT); before calling nsSVGDisplayContainerFrame::Init(); to make sure we set the correct bits before doing other things.

nsFrame::Init will end up copying the value of the NS_FRAME_IS_TEXT frame state bit from the parent frame (as it does with all frame state bits that inherit), so this needs to stay after our nsSVGDisplayContainerFrame::Init() call.

I think we can just call ScheduleReflowSVGNonDisplayText directly inside SVGTextFrame::Init if we are a non-display frame.

  1. Need to handle the case of removing/reinserting text frame in MaybeScheduleReflowSVGNonDisplayText() properly, especially when checking NS_FRAME_FIRST_REFLOW.

I can't remember whether the check for NS_FRAME_FIRST_REFLOW there is an optimization (since we should get around to reflowing the frame at some point anyway), or if there was a correctness reason.

Flags: needinfo?(cam)
Flags: needinfo?(boris.chiou)

(In reply to milchmann from comment #8)

Since this problem is kind of critical for us and we don't know how bugs are handled at Mozilla, I would like to ask if there is an ETA for a fix, or if the bug could be indefinitely on hold.

Sorry for not being able to get to this sooner. This will be fixed in Firefox 78.

Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3d8f0e488dbf
Reflow non-display SVG text that has just been inserted. r=longsonr
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
You need to log in before you can comment on or make changes to this bug.