Closed Bug 1480618 Opened 6 years ago Closed 6 years ago

Range APIs do not construct / move trees in tree order (observable by custom elements)

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Version:
63 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: bicknellr, Assigned: edgar)

References

Details

Attachments

(2 files)

The test page mentioned in the bug.
5.19 KB, text/html
Details
Bug 1480618 - Cloning node for Range APIs should be in tree order;
46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.75 Safari/537.36

Steps to reproduce:

1. Create a deep and wide tree of many custom elements with defined constructors, `connectedCallback`, and `disconnectedCallback`.
2. Create a range that such that the boundaries of the range are deep enough in the tree to cause many of the custom elements to be only partially contained within the range.
3. Call the range's `cloneContents` or `extractContents`.

(Specific cases listed below.)


Actual results:

Nodes in the cloned / extracted fragment are not created or moved in tree order:

1. During `extractContents`, nodes that are partially contained in the range are cloned into the created fragment, rather than being moved out of the original range (as expected). However, the order in which the clones / moves is now observably incorrect through the order of the custom element constructors and callbacks.

  To see this using the attached page:

  - Uncheck all checkboxes.
  - Click the "create source" button.
  - Check all checkboxes.
  - Click the "extract" button.

  You'll get a tree that looks like this:

  - [#2 NEW, C17]
      - [#1 NEW, C18]
          - [#0 NEW, C19] START.....)
          - [, D3, C20] (root-0-0-2)
      - [, D4, C21] (root-0-1)
          - [, D5, C22] (root-0-1-0)
          - [, D6, C23] (root-0-1-1)
          - [, D7, C24] (root-0-1-2)
  - [, D8, C25] (root-1)
  - [#9 NEW, C26] (root-2)
      - [, D10, C27] (root-2-0)
          - [, D11, C28] (root-2-0-0)
          - [, D12, C29] (root-2-0-1)
          - [, D13, C30] (root-2-0-2)
      - [#14 NEW, C31] (root-2-1)
          - [, D15, C32] (root-2-1-0)
          - [#16 NEW, C33] (root-2-1-1.....END

  (Sorry about the leading commas, they're not relevant.)

  In this tree, the constructors for the elements that were cloned as part of `extractContents` are not called in tree order. Particularly, notice that the constructors for the partially contained nodes in the 'first partially contained child' are called from the inside of the tree out. Also, notice that the removal of the inner node "(root-0-0-2)" (D3) happens after all of its cloned ancestors are cloned. (#1, #2, D3)

  In the spec section about extracting a range [1], step 14 describes how the 'first partially contained child' of the nearest containing ancestor of a range is handled. Particularly, it explicitly clones the partially contained child first and extracts the range inside of it next. This is also true of step 17, describing how the 'last partially contained child' is handled. Though, I think Firefox is handling the 'last partially contained child' case correctly.

2. During `cloneContents`, the newly created tree is not constructed in tree order.

  Using the attached page:

  - Check only the "#n" checkbox.
  - Click the "create source" button.
  - Click the "clone" button.

  You'll get a tree that looks like this:

  - [#22]
      - [#21]
          - [#20] START.....)
          - [#23] (root-0-0-2)
      - [#24] (root-0-1)
          - [#25] (root-0-1-0)
          - [#26] (root-0-1-1)
          - [#27] (root-0-1-2)
  - [#28] (root-1)
  - [#29] (root-2)
      - [#30] (root-2-0)
          - [#31] (root-2-0-0)
          - [#32] (root-2-0-1)
          - [#33] (root-2-0-2)
      - [#34] (root-2-1)
          - [#35] (root-2-1-0)
          - [#36] (root-2-1-1.....END

  All of the elements in the output tree are cloned. Like the steps for extracting a range [1], the steps for cloning a range [2] imply that the new tree should be constructed in tree order. So, the numbers in the new tree here should be in ascending order from top to bottom.

[1]: https://dom.spec.whatwg.org/#concept-range-extract
[2]: https://dom.spec.whatwg.org/#concept-range-clone


Expected results:

1. `extractContents` with logging for the constructors ("#n", "NEW") only on during the `extractContents` call (not when clicking "create source") should produce this tree:

- [#0 NEW, C17]
    - [#1 NEW, C18]
        - [#2 NEW, C19] START.....)
        - [, D3, C20] (root-0-0-2)
    - [, D4, C21] (root-0-1)
        - [, D5, C22] (root-0-1-0)
        - [, D6, C23] (root-0-1-1)
        - [, D7, C24] (root-0-1-2)
- [, D8, C25] (root-1)
- [#9 NEW, C26] (root-2)
    - [, D10, C27] (root-2-0)
        - [, D11, C28] (root-2-0-0)
        - [, D12, C29] (root-2-0-1)
        - [, D13, C30] (root-2-0-2)
    - [#14 NEW, C31] (root-2-1)
        - [, D15, C32] (root-2-1-0)
        - [#16 NEW, C33] (root-2-1-1.....END

2. `cloneContents` with logging for the constructors should produce this tree:

  - [#20]
      - [#21]
          - [#22] START.....)
          - [#23] (root-0-0-2)
      - [#24] (root-0-1)
          - [#25] (root-0-1-0)
          - [#26] (root-0-1-1)
          - [#27] (root-0-1-2)
  - [#28] (root-1)
  - [#29] (root-2)
      - [#30] (root-2-0)
          - [#31] (root-2-0-0)
          - [#32] (root-2-0-1)
          - [#33] (root-2-0-2)
      - [#34] (root-2-1)
          - [#35] (root-2-1-0)
          - [#36] (root-2-1-1.....END

  (All will have "NEW", if that box is checked.)
This is reproducible using the steps and attachment in comment 0.  I tested this in Windows 10 with the latest nightly.

reporter (blcknellr), is this an enhancement or a regression bug? 

--- test Environment ---
Version 	63.0a1
Build ID 	20180807100107
Update Channel 	nightly
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM
Ever confirmed: true
Flags: needinfo?(bicknellr)
Product: Firefox → Core
I think this would be considered an enhancement, given that custom elements haven't shipped to stable yet. I haven't tested yet but I wonder if these ordering problems are observable with a MutationObserver too.
Flags: needinfo?(bicknellr)
Severity: normal → enhancement
(I made this to block bug 1205323 just for tracking purposing. Not sure yet whether this needs to block, probably not.)
Did you mean to block bug 889230 (custom elements) instead of the Shadow DOM one?
Hmm, doesn't matter much, but yeah, perhaps I could move it there.
Blocks: custom-elements-initial-release
No longer blocks: shadowdom-initial-release
Flags: needinfo?(echen)
I took a quick look and it seems because we clone the ancestors of start node from the inside of the tree out [1].

[1] https://searchfox.org/mozilla-central/rev/0f4d50ff5211e8dd85e119ef683d04b63062ed90/dom/base/nsRange.cpp#2352-2400
it might be easier to fix this by reordering CE callback calls. might.
Not sure if reordering upgrade reactions would be easier. I reorder the ancestors cloning in nsRange::CloneParentsBetween and then I could get callbacks in tree order. But not sure if there will be any side effect, let's wait the try result first, https://treeherder.mozilla.org/#/jobs?repo=try&revision=29068ad406105c706e8367d979c84681708f56a3&filter-tier=1&group_state=expanded.
Assignee: nobody → echen
Flags: needinfo?(echen)
Please upgrade to P2 if appropriate.
Priority: -- → P3
Priority: P3 → P2
Comment on attachment 8999639 [details]
Bug 1480618 - Cloning node for Range APIs should be in tree order;

Olli Pettay [:smaug] has approved the revision.
Attachment #8999639 - Flags: review+
Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8fdd88cea188
Cloning node for Range APIs should be in tree order; r=smaug
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12478 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/8fdd88cea188
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: