Implement element.innerText getter #19754

ferjm · 2018-01-12T18:33:44Z

This change is

highfive · 2018-01-12T18:33:47Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/webidls/HTMLElement.webidl, components/script/dom/node.rs, components/script/dom/htmlelement.rs, components/script/dom/window.rs
@KiChjang: components/script/dom/webidls/HTMLElement.webidl, components/script/dom/node.rs, components/script/dom/htmlelement.rs, components/script/dom/window.rs
@fitzgen: components/script/dom/webidls/HTMLElement.webidl, components/script/dom/node.rs, components/script/dom/htmlelement.rs, components/script/dom/window.rs
@emilio: components/layout/query.rs

highfive · 2018-01-12T18:33:49Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify layout and script code, but no tests are modified. Please consider adding a test!

ferjm · 2018-01-12T18:34:27Z

Since I'd like to start learning about how layout works, I figured that I could get back to this implementation. I made some progress wrt my previous attempt, but I still couldn't figure out how to get the text fragments from each node's LayoutData.

@mbrubeck , I would really appreciate if you could give me some pointers about how to get this data. Thank you in advance for your help!

mbrubeck · 2018-01-12T21:36:52Z

There are two types of text fragments: UnscannedText and ScannedText, which are created at different points during flow construction. The ScannedText fragments are transformed based
are transformed based on the white-space and text-transform styles, so I think they are what you want to access here. Unfortunately the ConstructionResult will only contain the initial unscanned text fragments, so we'll need to find a different way to access the scanned text. I'm still investigating how to do this...

bors-servo · 2018-01-28T12:43:09Z

☔ The latest upstream changes (presumably #19881) made this pull request unmergeable. Please resolve the merge conflicts.

ferjm · 2018-02-08T17:50:08Z

I made some progress here. Instead of trying to get the text from the node's LayoutData I got it from the display list. The result is still not the expected one, but at least I am able to show some text now.

Servo:

Firefox (showing expected output):

ferjm · 2018-02-09T11:46:53Z

Looking better now:

ferjm · 2018-02-12T11:16:22Z

r? @mbrubeck

There are still a bunch of tests failing and the tables stuff pending, but I think this is a start.

emilio · 2018-02-12T11:41:43Z

components/layout/query.rs

+// https://html.spec.whatwg.org/multipage/#inner-text-collection-steps
+#[allow(unsafe_code)]
+fn inner_text_collection_steps<N: LayoutNode>(node: N,
+                                              display_list: &Option<Arc<DisplayList>>,


nit: Given you bail out here, you may as well bail out on the parent and get a &DisplayList here, right?

emilio · 2018-02-12T11:41:43Z

components/layout/query.rs

+                // Step 4.
+                let opaque_child = child.opaque();
+                for item in &display_list.as_ref().unwrap().list {
+                    if item.base().metadata.node != opaque_child {


This looks... not great, this traverses the whole display list for the page for each text-node, right?

emilio · 2018-02-12T11:41:43Z

components/layout/query.rs

+                        continue;
+                    }
+                    match item {
+                        &DisplayItem::Text(ref text) => {


nit: if let.

emilio · 2018-02-12T11:41:43Z

components/layout/query.rs

+}
+
+fn is_block_level_or_table_caption(display: &Display) -> bool {
+    #[cfg(feature = "gecko")]


nit: No need for feature = "gecko" stuff on layout.

emilio · 2018-02-12T11:41:43Z

components/script/dom/htmlelement.rs

+        let style = window.GetComputedStyle(element, None);
+
+        // Step 1.
+        let element_not_rendered = !node.is_in_doc() || style.GetPropertyValue(DOMString::from("display")) == "none";


nit: Please avoid using GetComputedStyle, use has_css_layout_box instead. This is completely bogus otherwise.

mbrubeck · 2018-02-12T18:38:58Z

components/layout/query.rs

+    let mut text = HashMap::new();
+    for item in &display_list.as_ref().list {
+        if let &DisplayItem::Text(ref text_content) = item {
+            let entries = text.entry(&item.base().metadata.node).or_insert(Vec::new());


The bad news is that #20031 conflicts with this, because it moves the text out of the display list. The good news is that it should actually make it easier to query for the text of a node, because the text will already be in a HashMap, so you won't need to build one here...

mbrubeck · 2018-02-12T18:57:53Z

@bors-servo r+

Approving this because it looks ready to land now. If it lands before #20031, then #20031 can resolve the conflict by making some fairly straightforward changes to the code that fetches text from the display list. If this doesn't manage to land right away and #20031 ends up landing first, then that conflict resolution will need to be done in this PR instead.

bors-servo · 2018-02-12T18:57:53Z

📌 Commit d988c3d has been approved by mbrubeck

bors-servo · 2018-02-12T18:57:59Z

⌛ Testing commit d988c3d with merge 08a4ea4...


        Auto merge of #19754 - ferjm:innertext, r=mbrubeck

Implement element.innerText getter  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19754)

bors-servo · 2018-02-12T19:27:56Z

💔 Test failed - mac-rel-wpt3

ferjm · 2018-02-12T22:30:42Z

@bors-servo retry

bors-servo · 2018-02-12T22:52:40Z

⌛ Testing commit d279bac with merge 1aaa309...


        Auto merge of #19754 - ferjm:innertext, r=mbrubeck

Implement element.innerText getter  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19754)

bors-servo · 2018-02-12T23:21:44Z

💔 Test failed - linux-rel-wpt

mbrubeck · 2018-02-13T01:33:54Z

There are two newly-passing subtests in /html/dom/interfaces.html:

{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "HTMLElement interface: attribute innerText", "test": "/html/dom/interfaces.html", "line": 27384, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "HTMLElement interface: document.createElement(\"noscript\") must inherit property \"innerText\" with the proper type", "test": "/html/dom/interfaces.html", "line": 27470, "action": "test_result", "expected": "FAIL"}


        Implement element.innerText getter

emilio · 2018-02-13T08:40:47Z

@bors-servo delegate+

bors-servo · 2018-02-13T08:40:48Z

✌️ @ferjm can now approve this pull request

ferjm · 2018-02-13T08:41:45Z

@bors-servo r=mbrubeck

bors-servo · 2018-02-13T08:41:46Z

📌 Commit 2a4535f has been approved by mbrubeck

bors-servo · 2018-02-13T08:41:52Z

⌛ Testing commit 2a4535f with merge 9e64008...


        Auto merge of #19754 - ferjm:innertext, r=mbrubeck

Implement element.innerText getter  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19754)

bors-servo · 2018-02-13T09:32:52Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: mbrubeck
Pushing 9e64008 to master...


        Implement element.innerText getter

Upstreamed from servo/servo#19754 [ci skip]

highfive assigned emilio Jan 12, 2018

highfive added the S-awaiting-review label Jan 12, 2018

highfive added the S-needs-rebase label Jan 28, 2018

ferjm force-pushed the ferjm:innertext branch from bcbb647 to ed9ce2c Feb 8, 2018

ferjm force-pushed the ferjm:innertext branch from ed9ce2c to 0ec7438 Feb 9, 2018

jdm removed the S-needs-rebase label Feb 9, 2018

ferjm force-pushed the ferjm:innertext branch 2 times, most recently from 6143e93 to 4bee985 Feb 12, 2018

highfive assigned mbrubeck and unassigned emilio Feb 12, 2018

ferjm changed the title ~~WIP - Implement element.innerText getter~~ Implement element.innerText getter Feb 12, 2018

emilio reviewed Feb 12, 2018

View changes

ferjm force-pushed the ferjm:innertext branch from 4bee985 to d988c3d Feb 12, 2018

mbrubeck mentioned this pull request Feb 12, 2018

Reorganise display list builder #20031

Merged

mbrubeck reviewed Feb 12, 2018

View changes

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 12, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 12, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Feb 12, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 12, 2018

Implement element.innerText getter

Loading status checks…

2a4535f

ferjm force-pushed the ferjm:innertext branch from d279bac to 2a4535f Feb 13, 2018

highfive added S-awaiting-review and removed S-tests-failed labels Feb 13, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 13, 2018

bors-servo merged commit 2a4535f into servo:master Feb 13, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Feb 13, 2018

ferjm deleted the ferjm:innertext branch Feb 13, 2018

servo-wpt-sync mentioned this pull request Feb 14, 2018

Implement element.innerText getter web-platform-tests/wpt#9528

Merged

jdm added a commit to web-platform-tests/wpt that referenced this pull request Feb 14, 2018

Implement element.innerText getter

d202d15

Upstreamed from servo/servo#19754 [ci skip]

ferjm mentioned this pull request Feb 15, 2018

Implement element.innerText #16107

Closed

servo / servo

Implement element.innerText getter #19754

Implement element.innerText getter #19754

ferjm commented Jan 12, 2018 •

edited by SimonSapin

highfive commented Jan 12, 2018

highfive commented Jan 12, 2018

ferjm commented Jan 12, 2018

mbrubeck commented Jan 12, 2018

bors-servo commented Jan 28, 2018

ferjm commented Feb 8, 2018 •

edited

ferjm commented Feb 9, 2018

ferjm commented Feb 12, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mbrubeck commented Feb 12, 2018

bors-servo commented Feb 12, 2018

bors-servo commented Feb 12, 2018

bors-servo commented Feb 12, 2018

ferjm commented Feb 12, 2018

bors-servo commented Feb 12, 2018

bors-servo commented Feb 12, 2018

mbrubeck commented Feb 13, 2018

emilio commented Feb 13, 2018

bors-servo commented Feb 13, 2018

ferjm commented Feb 13, 2018

bors-servo commented Feb 13, 2018

bors-servo commented Feb 13, 2018

bors-servo commented Feb 13, 2018

servo / servo

Join GitHub today

Implement element.innerText getter #19754

Implement element.innerText getter #19754

Conversation

ferjm commented Jan 12, 2018 • edited by SimonSapin

highfive commented Jan 12, 2018

highfive commented Jan 12, 2018

ferjm commented Jan 12, 2018

mbrubeck commented Jan 12, 2018

bors-servo commented Jan 28, 2018

ferjm commented Feb 8, 2018 • edited

ferjm commented Feb 9, 2018

ferjm commented Feb 12, 2018

This comment has been minimized.

emilio Feb 12, 2018

This comment has been minimized.

emilio Feb 12, 2018

This comment has been minimized.

emilio Feb 12, 2018

This comment has been minimized.

emilio Feb 12, 2018

This comment has been minimized.

emilio Feb 12, 2018

This comment has been minimized.

mbrubeck Feb 12, 2018

mbrubeck commented Feb 12, 2018

bors-servo commented Feb 12, 2018

bors-servo commented Feb 12, 2018

bors-servo commented Feb 12, 2018

ferjm commented Feb 12, 2018

bors-servo commented Feb 12, 2018

bors-servo commented Feb 12, 2018

mbrubeck commented Feb 13, 2018

emilio commented Feb 13, 2018

bors-servo commented Feb 13, 2018

ferjm commented Feb 13, 2018

bors-servo commented Feb 13, 2018

bors-servo commented Feb 13, 2018

bors-servo commented Feb 13, 2018

ferjm commented Jan 12, 2018 •

edited by SimonSapin

ferjm commented Feb 8, 2018 •

edited