Skip to content

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

Merged
zcorpan merged 5 commits into
w3c:masterfrom
SimonSapin:cssomstring
Apr 21, 2017
Merged

[cssom][all] Add CSSOMString and replace DOMString usage with it#1266
zcorpan merged 5 commits into
w3c:masterfrom
SimonSapin:cssomstring

Conversation

@SimonSapin

@SimonSapin SimonSapin commented Apr 21, 2017

Copy link
Copy Markdown
Contributor

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 Current Work label Apr 21, 2017
Comment thread cssom/Overview.bs

Or, alternatively:

<pre class="def lang-webidl">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use class="idl extract" here instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With that Bikeshed gives:

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... @tabatkins should this work?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Comment thread cssom/Overview.bs Outdated
This choice effectively allows implementations to do this replacement,
but does not require it.

This enables an implementation to use UTF-8 internally to represent strings in memory.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/This/Using {{USVString}}/

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment thread cssom/Overview.bs Outdated

On the other hand,
implementations that internally represent strings as 16-bit <a>code units</a>
may prefer to avoid the cost of doing this replacement.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use may in a note

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced with “might”.

@zcorpan

zcorpan commented Apr 21, 2017

Copy link
Copy Markdown
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?

@zcorpan

zcorpan commented Apr 21, 2017

Copy link
Copy Markdown
Contributor

(Thanks for fixing link errors in cssom!)

@SimonSapin

Copy link
Copy Markdown
Contributor Author
  • 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

zcorpan commented Apr 21, 2017

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

Changed Stylesheet::title back to DOMString?.

CSSWG resolution:
w3c#1217 (comment)

Fix w3c#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 commented Apr 21, 2017

Copy link
Copy Markdown
Contributor

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

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

Labels

cssom-1 Current Work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants