Closed Bug 1472169 Opened 6 years ago Closed 11 months ago

<textarea> validity state is not preserved correctly after a clone.

Categories

(Core :: DOM: Core & HTML, defect, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
116 Branch
Tracking Flags:
Tracking Status
firefox-esr102 --- wontfix
firefox114 --- wontfix
firefox115 --- wontfix
firefox116 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

(Keywords: topcrash)

Crash Data

Attachments

(2 files, 1 obsolete file)

test.html
401 bytes, text/html
Details
Bug 1472169 - Notify of appends from CloneAndAdopt clone path. r=smaug
48 bytes, text/x-phabricator-request
Details | Review
Attached file test.html — 
This is the underlying cause of bug 1472027.

See the test-case (should alert the same twice).

The reason for this is that CloneAndAdopt calls AppendChild with aNotify == false. This makes HTMLTextAreaElement::ContentAppended to not be invoked, and thus the validity state to not be updated correctly.

Probably HTMLInputElement is also affected, though I haven't checked.

We'll update the validity state whenever we recompute it, in the case of bug 1472027 when BindToTree is called, from the frame constructor. That is, not good.
Should we notify from CloneAndAdopt? Looks like it shouldn't be too harfmul...
This should be fine I think, given we should notify only to the cloned subtree.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42033fa7b497a8baf0f9132a8d3906e27b04f76d

I would expect a couple other CSS failures from other patches in my queue, but nothing catastrophic.
Attachment #8988724 - Flags: review?(bugs)
Assignee: nobody → emilio
> We'll update the validity state whenever we recompute it, in the case of bug 1472027 when BindToTree is called

This can happen for other reasons too, right?  For example, consider this case:

  <!doctype html>
  <style>
    textarea:invalid { border: 5px solid red; }
  </style>
  <fieldset disabled>
    <textarea required></textarea>
  </fieldset>
  <script>
    onload = function() {
      document.body.offsetWidth;
      var t = document.querySelector("textarea");
      var f = document.querySelector("fieldset");
      f.appendChild(t.cloneNode(true));
    }
  </script>

in which case the textarea is invalid when returned from cloneNode but becomes valid (due to being disabled) once it's appended to the fieldset.
> Probably HTMLInputElement is also affected, though I haven't checked.

HTMLInputElement validity does not depend on kids, so no.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> > We'll update the validity state whenever we recompute it, in the case of bug 1472027 when BindToTree is called
> 
> This can happen for other reasons too, right?  For example, consider this
> case:
> 
>   <!doctype html>
>   <style>
>     textarea:invalid { border: 5px solid red; }
>   </style>
>   <fieldset disabled>
>     <textarea required></textarea>
>   </fieldset>
>   <script>
>     onload = function() {
>       document.body.offsetWidth;
>       var t = document.querySelector("textarea");
>       var f = document.querySelector("fieldset");
>       f.appendChild(t.cloneNode(true));
>     }
>   </script>
> 
> in which case the textarea is invalid when returned from cloneNode but
> becomes valid (due to being disabled) once it's appended to the fieldset.

Sure, but that's not concerning re(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> > We'll update the validity state whenever we recompute it, in the case of bug 1472027 when BindToTree is called
> 
> This can happen for other reasons too, right?  For example, consider this
> case:
> 
>   <!doctype html>
>   <style>
>     textarea:invalid { border: 5px solid red; }
>   </style>
>   <fieldset disabled>
>     <textarea required></textarea>
>   </fieldset>
>   <script>
>     onload = function() {
>       document.body.offsetWidth;
>       var t = document.querySelector("textarea");
>       var f = document.querySelector("fieldset");
>       f.appendChild(t.cloneNode(true));
>     }
>   </script>
> 
> in which case the textarea is invalid when returned from cloneNode but
> becomes valid (due to being disabled) once it's appended to the fieldset.

Sure, but in the <svg:use> case the <textarea> would belong to a different anonymous subtree, so I don't think we look past the anonymous <use> shadow tree root.
(got mid-air'ed)
This does regress, as an example, cloning of single page HTML spec document ~25%.
And we do clone documents for printing and print preview.

And this would make bug 1232023 possibly worse (locally I'm getting similar numbers in Nightly and Chrome using testcase in that bug).
Comment on attachment 8988724 [details]
Bug 1472169: Notify of appends from CloneAndAdopt clone path. r=smaug

Because of the performance implications, I'm not ready to take this patch without some more investigation whether this could be fixed in other ways.
Attachment #8988724 - Flags: review?(bugs)
And apparently bz had commented about performance in phabricator.
Uh, I hate this split model for discussions, and phabricator forces to that :/
(In reply to Olli Pettay [:smaug] from comment #9)
> And apparently bz had commented about performance in phabricator.
> Uh, I hate this split model for discussions, and phabricator forces to that
> :/

Phabriactor has an API where you can automatically post any comments in Phab Diff to the associated bug (we did this at my last company with JIRA integration). This can likely be done with Bugzilla integration.
Flags: needinfo?(mcote)
Priority: -- → P2
(In reply to Marion Daly [:mdaly] from comment #10)
> (In reply to Olli Pettay [:smaug] from comment #9)
> > And apparently bz had commented about performance in phabricator.
> > Uh, I hate this split model for discussions, and phabricator forces to that
> > :/
> 
> Phabriactor has an API where you can automatically post any comments in Phab
> Diff to the associated bug (we did this at my last company with JIRA
> integration). This can likely be done with Bugzilla integration.

Based on lessons we learned from MozReview, we've explicitly decided not to do this for a variety of reasons, including

* syncing is hard given the threading, markup, and various other features that differ between Phabricator and BMO
* the discussion still gets forked, as some people reply to Phabricator and others reply only in BMO
* we want code reviews to live in Phabricator, not for Phabricator to just be an extension to BMO

See also lengthy discussion in various newsgroup threads, including

https://groups.google.com/forum/#!topic/mozilla.tools/4qroY2Iia9I
https://groups.google.com/d/msg/mozilla.dev.platform/Y8kInYxo8UU/z1VZIzaSBAAJ
https://groups.google.com/d/msg/mozilla.dev.platform/V1vuWPeD_hc/5Nha-8iXAwAJ
Flags: needinfo?(mcote)
Blocks: 1450181

The following patch is waiting for review from an inactive reviewer:

ID Title Author Reviewer Status
D1878 Bug 1472169: Notify of appends from CloneAndAdopt clone path. r=smaug emilio smaug: Resigned from review

:emilio, could you please find another reviewer or abandon the patch if it is no longer relevant?

For more information, please visit auto_nag documentation.

Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Severity: normal → S3
Blocks: 1836854

So per discussion, three approaches:

  • The one in the patch. Pros: Simpler, consistent. Cons: Potentially more work / slower.
  • Reuse DoneAddingChildren. We could do that by:
    • Manually setting mDoneAddingChildren = false in CopyInnerTo / Clone. Pros: Potentially faster. Cons: A bit inconsistent / harder to follow.
    • Changing mozilla::dom::FromParser to something like mozilla::dom::CreationMode. Pros: Potentially faster. Cons: More churn.

I'm happy to do (1) or (2) or (3). For now let's try (1) and see what benchmarks say?

Attachment #8988724 - Attachment is obsolete: true
Duplicate of this bug: 1836854
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/badb93aa042a
Notify of appends from CloneAndAdopt clone path. r=smaug

Copying crash signatures from duplicate bugs.

Crash Signature: [@ mozilla::RestyleManager::ElementStateChanged]

The bug is linked to a topcrash signature, which matches the following criterion:

  • Top 10 AArch64 and ARM crashes on nightly

:emilio, could you consider increasing the severity of this top-crash bug?

For more information, please visit BugBot documentation.

Flags: needinfo?(emilio)
Keywords: topcrash
Flags: needinfo?(emilio)

https://hg.mozilla.org/mozilla-central/rev/badb93aa042a

Status: NEW → RESOLVED
Closed: 11 months ago
status-firefox116: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 116 Branch

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox115 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(emilio)

Very long-standing bug.

status-firefox115: affected → wontfix
Flags: needinfo?(emilio)
See Also: → 1843445
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: