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

[cssom][all] Add CSSOMString and replace DOMString usage with it #1266

Merged
merged 5 commits into from Apr 21, 2017

Conversation

Projects
None yet
3 participants
@SimonSapin
Contributor

SimonSapin commented Apr 21, 2017

CSSOMString is an IDL typedef of either USVString or DOMString, chosen by implementations.

CSSWG resolution: #1217 (comment). Fixes #1217.

Each replaced occurrence is one of:

  • CSS syntax
  • A name (for example a property name) that also occurs in CSS syntax
  • Stylesheet::type, which is always text/css.
  • Stylesheet::title, which is set from the eponymous HTML content attribute of <style> and <link> elements.

@SimonSapin SimonSapin added the cssom-1 label Apr 21, 2017

Or, alternatively:
<pre class="def lang-webidl">

This comment has been minimized.

@zcorpan

zcorpan Apr 21, 2017

Member

Can use class="idl extract" here instead.

@zcorpan

zcorpan Apr 21, 2017

Member

Can use class="idl extract" here instead.

This comment has been minimized.

@SimonSapin

SimonSapin Apr 21, 2017

Contributor

With that Bikeshed gives:

FATAL ERROR: Multiple local 'typedef' <dfn>s have the same linking text 'CSSOMString'.
@SimonSapin

SimonSapin Apr 21, 2017

Contributor

With that Bikeshed gives:

FATAL ERROR: Multiple local 'typedef' <dfn>s have the same linking text 'CSSOMString'.

This comment has been minimized.

@zcorpan

zcorpan Apr 21, 2017

Member

Hmm... @tabatkins should this work?

@zcorpan

zcorpan Apr 21, 2017

Member

Hmm... @tabatkins should this work?

This comment has been minimized.

@tabatkins

tabatkins Apr 28, 2017

Member

I haven't added the "extract" support yet. (The bug is still open!)

(And in any case, I think it's probably better to do exactly this - instead of claiming something is WebIDL and then opting out, just ask it to be highlighted as WebIDL but otherwise be meaningless text.)

@tabatkins

tabatkins Apr 28, 2017

Member

I haven't added the "extract" support yet. (The bug is still open!)

(And in any case, I think it's probably better to do exactly this - instead of claiming something is WebIDL and then opting out, just ask it to be highlighted as WebIDL but otherwise be meaningless text.)

Show outdated Hide outdated cssom/Overview.bs Outdated
Show outdated Hide outdated cssom/Overview.bs Outdated
@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Apr 21, 2017

Member
  • Stylesheet::title, which is set from the eponymous HTML content attribute of <style> and <link> elements.

Can you explain why this one shouldn't be DOMString?

Member

zcorpan commented Apr 21, 2017

  • Stylesheet::title, which is set from the eponymous HTML content attribute of <style> and <link> elements.

Can you explain why this one shouldn't be DOMString?

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Apr 21, 2017

Member

(Thanks for fixing link errors in cssom!)

Member

zcorpan commented Apr 21, 2017

(Thanks for fixing link errors in cssom!)

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Apr 21, 2017

Contributor
  • Stylesheet::title, which is set from the eponymous HTML content attribute of <style> and <link> elements.

Can you explain why this one shouldn't be DOMString?

I did hesitate about this one, it just seemed more “within style system territory”. I don’t feel strongly about it, though. In Stylo we can (and possibly already do) keep this on the C++ side rather than the Rust side of the bindings. (And Servo needs to decide what to do about every DOMString, not just stylesheet titles.)

Contributor

SimonSapin commented Apr 21, 2017

  • Stylesheet::title, which is set from the eponymous HTML content attribute of <style> and <link> elements.

Can you explain why this one shouldn't be DOMString?

I did hesitate about this one, it just seemed more “within style system territory”. I don’t feel strongly about it, though. In Stylo we can (and possibly already do) keep this on the C++ side rather than the Rust side of the bindings. (And Servo needs to decide what to do about every DOMString, not just stylesheet titles.)

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Apr 21, 2017

Member

So title on the HTML side is on HTMLElement and is DOMString. Then "create a CSS stylesheet" the title content attribute is passed for the title, which is then updated if the content attribute is changed. styleSheet.title then returns that value... which is a DOMString.

I'm open to changing how this works, but since the string will exist as a DOMString anyway, it seems kinda pointless to use CSSOMString on StyleSheet.

Member

zcorpan commented Apr 21, 2017

So title on the HTML side is on HTMLElement and is DOMString. Then "create a CSS stylesheet" the title content attribute is passed for the title, which is then updated if the content attribute is changed. styleSheet.title then returns that value... which is a DOMString.

I'm open to changing how this works, but since the string will exist as a DOMString anyway, it seems kinda pointless to use CSSOMString on StyleSheet.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Apr 21, 2017

Contributor

Changed Stylesheet::title back to DOMString?.

Contributor

SimonSapin commented Apr 21, 2017

Changed Stylesheet::title back to DOMString?.

[all] Replace most occurrences DOMString with CSSOMString.
CSSWG resolution:
#1217 (comment)

Fix #1217.

Each occurrence is one of:

* CSS syntax
* A name (for example a property name) that also occurs in CSS syntax
* `Stylesheet::type`, which is always `text/css`.

Not replaced:

* `Stylesheet::title`, which is set from the eponymous HTML content attribute
  of [`<style>`](https://html.spec.whatwg.org/multipage/semantics.html#attr-style-title)
  and [`<link>`](https://html.spec.whatwg.org/multipage/semantics.html#attr-link-title)
  elements.
  These contant attributes are reflected as `HTMLElement::title` DOM attributes,
  where they are `DOMString`.

@zcorpan zcorpan merged commit 4049ec2 into w3c:master Apr 21, 2017

@zcorpan zcorpan referenced this pull request Apr 21, 2017

Open

Test CSSOMString #5641

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Apr 21, 2017

Member

Filed web-platform-tests/wpt#5641 to follow up on tests. The href change affects everyone, so might also need bugs on browsers.

Member

zcorpan commented Apr 21, 2017

Filed web-platform-tests/wpt#5641 to follow up on tests. The href change affects everyone, so might also need bugs on browsers.

@SimonSapin SimonSapin deleted the SimonSapin:cssomstring branch May 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment