Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement :placeholder-shown (fixes #10561) #11572

Merged
merged 1 commit into from Jun 7, 2016

Conversation

@nox
Copy link
Member

nox commented Jun 3, 2016

This change is Reviewable

@highfive
Copy link

highfive commented Jun 3, 2016

Heads up! This PR modifies the following files:

  • @bholley: components/style/element_state.rs, components/style/selector_impl.rs
  • @KiChjang: components/script/dom/element.rs, components/script/dom/htmlinputelement.rs, components/script/textinput.rs
@highfive
Copy link

highfive commented Jun 3, 2016

warning Warning warning

  • These commits modify style, layout, and script code, but no tests are modified. Please consider adding a test!
@nox
Copy link
Member Author

nox commented Jun 3, 2016

@highfive highfive assigned SimonSapin and unassigned glennw Jun 3, 2016
@nox
Copy link
Member Author

nox commented Jun 3, 2016

Err, forgot textarea...

@nox nox force-pushed the nox:placeholder-shown branch from aa1f644 to 8f8ba19 Jun 3, 2016
@highfive
Copy link

highfive commented Jun 3, 2016

New code was committed to pull request.

@nox nox force-pushed the nox:placeholder-shown branch from 8f8ba19 to 4227283 Jun 3, 2016
@highfive
Copy link

highfive commented Jun 3, 2016

New code was committed to pull request.

@nox
Copy link
Member Author

nox commented Jun 3, 2016

Just realised that textarea doesn't support placeholder in Servo yet anyway, so this is ready to be reviewed.

@nox
Copy link
Member Author

nox commented Jun 3, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2016

Trying commit 4227283 with merge ae30947...

bors-servo added a commit that referenced this pull request Jun 3, 2016
Implement :placeholder-shown (fixes #10561)

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11572)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2016

💔 Test failed - linux-dev

@nox nox force-pushed the nox:placeholder-shown branch from 4227283 to ff899dc Jun 3, 2016
@highfive
Copy link

highfive commented Jun 3, 2016

New code was committed to pull request.

@highfive highfive removed the S-tests-failed label Jun 3, 2016
@nox
Copy link
Member Author

nox commented Jun 3, 2016

Fixed the geckolib failure.

@SimonSapin
Copy link
Member

SimonSapin commented Jun 6, 2016

Reviewed 2 of 6 files at r1, 4 of 4 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


components/script/dom/htmlinputelement.rs, line 721 [r4] (raw file):

        let has_value = !self.textinput.borrow().is_empty();
        let el = self.upcast::<Element>();
        el.set_placeholder_shown_state(has_placeholder && !has_value);

Shouldn’t this logic involve "the element has focus" too? Can it be de-duplicated with whatever code decides to show the placholder or not?


ports/geckolib/wrapper.rs, line 355 [r4] (raw file):

    fn get_state(&self) -> ElementState {
        unsafe {
            ElementState::from_bits_truncate(Gecko_ElementState(self.element) as u16)

@bholley @heycam @emilio, This changes ElementState’s representation from u8 to u16. Do you need to change something on the Gecko side for this?


Comments from Reviewable

@emilio
Copy link
Member

emilio commented Jun 6, 2016

It seems to me that there shouldn't be a problem with the gecko side of this PR.

Gecko uses a 64 bit integer to represent state, and in the servo case we crop it to an u8. So probably at some point we might need to expand it more, but it should keep working as it was with this patch.

@SimonSapin
Copy link
Member

SimonSapin commented Jun 6, 2016

Alright, thanks Emilio.

(Discussed my other comment with @nox on IRC.)

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jun 6, 2016

📌 Commit ff899dc has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Jun 6, 2016

Testing commit ff899dc with merge 3c50368...

bors-servo added a commit that referenced this pull request Jun 6, 2016
Implement :placeholder-shown (fixes #10561)

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11572)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 7, 2016

💔 Test failed - linux-rel

@highfive
Copy link

highfive commented Jun 7, 2016

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-abspos-002.htm
  └   → /css-transforms-1_dev/html/transform-abspos-002.htm 71f0313eedfbfcce0b0fcc5ae55f34b1daa3b8d8
/css-transforms-1_dev/html/reference/transform-abspos-ref.htm 78d197606924062e8dd2a773c977afcecf8940f8
Testing 71f0313eedfbfcce0b0fcc5ae55f34b1daa3b8d8 == 78d197606924062e8dd2a773c977afcecf8940f8
@cbrewster
Copy link
Member

cbrewster commented Jun 7, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 7, 2016

Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows are reusable. Rebuilding only linux-rel...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 7, 2016

💔 Test failed - linux-rel

@highfive
Copy link

highfive commented Jun 7, 2016

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-table-007.htm
  └   → /css-transforms-1_dev/html/transform-table-007.htm a5c014b20ef1363bea6f24eda28c7efb7c45698a
/css-transforms-1_dev/html/reference/transform-blank-ref.htm fa6407b1acbbfea27e27061e7d1bdeca98e4a728
Testing a5c014b20ef1363bea6f24eda28c7efb7c45698a == fa6407b1acbbfea27e27061e7d1bdeca98e4a728
@cbrewster
Copy link
Member

cbrewster commented Jun 7, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 7, 2016

Testing commit ff899dc with merge 0f1f99a...

bors-servo added a commit that referenced this pull request Jun 7, 2016
Implement :placeholder-shown (fixes #10561)

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11572)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 7, 2016

@bors-servo bors-servo merged commit ff899dc into servo:master Jun 7, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.