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

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[milchmann]

Attachment: 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.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 2

[Erik Nordin [:nordzilla]]

I ran mozregression and found that the behavior changed in the following commit:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=baffa8a7b9618e9a02e4bd808adb71dead99cce5&tochange=71e6f3199f98ea08f93ed6f5a5cfab9ca5ca221d

Comment 3

[Erik Nordin [:nordzilla]]

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?

Comment 4

[Boris Chiou [:boris]]

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

Comment 5

[Boris Chiou [:boris]]

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.

Comment 6

[Boris Chiou [:boris]]

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

Comment 7

[Boris Chiou [:boris]]

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.

Comment 8

[milchmann]

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.

Comment 9

[Boris Chiou [:boris]]

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

Comment 10

[Cameron McCormack (:heycam)]

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.

2. 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.

Comment 11

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=03cdba4378305c91d06c9d3f999f741a0ac40787

Comment 12

[Cameron McCormack (:heycam)]

Attachment: Bug 1609663 - Reflow non-display SVG text that has just been inserted.

Comment 13

[Cameron McCormack (:heycam)]

(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.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d8f0e488dbf
Reflow non-display SVG text that has just been inserted. r=longsonr

Comment 15

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/3d8f0e488dbf