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

Support rb and rtc elements in the parser (ruby) #101

Closed
wants to merge 0 commits into from
Closed

Conversation

annevk
Copy link
Member

@annevk annevk commented Sep 4, 2015

Apart from the specific error messages for end-of-file and “body” &
“html” end tags, this matches what WebKit and Blink have implemented.
Only Blink implements these as HTMLUnknownElement.

@annevk
Copy link
Member Author

annevk commented Sep 4, 2015

@domenic
Copy link
Member

domenic commented Sep 4, 2015

Can we get a bit more detail on the compatibility? I'm specifically wondering:

  • What do Edge and Gecko implement?
  • What interfaces do they use?

Happy to run e.g. live-dom-viewer test cases.

@annevk
Copy link
Member Author

annevk commented Sep 4, 2015

Possible article problems:
103720:   <code>p</code> element, an <code>rb</code> element, an <code>rp</code> element, an <code>rt</code>
103721:   element, or an <code>rtc</code> element, the UA must pop the <span>current node</span> off the
103732:   <code>p</code> element, an <code>rb</code> element, an <code>rp</code> element, an <code>rt</code>
103733:   element, an <code>rtc</code> element, a <code>tbody</code> element, a <code>td</code> element, a

Not sure why this is suddenly a problem.

@domenic
Copy link
Member

domenic commented Sep 4, 2015

Maybe it only checks changed lines?

@annevk
Copy link
Member Author

annevk commented Sep 4, 2015

No, build.sh actually whitelists rt and rp. So for this change we need to add rb and rtc to the script. Wonderful.

@annevk
Copy link
Member Author

annevk commented Sep 4, 2015

Firefox uses HTMLElement using <rtc><script>w(document.querySelector("rtc"))</script> as input to Live DOM Viewer. I don't have Edge.

@annevk
Copy link
Member Author

annevk commented Sep 4, 2015

Apart from the class question, there's also:

  • Should these elements be conforming? Based on @Hixie's comments in that bug, I think they probably shouldn't. Although clearly there's also folks that think they should be. Also, I'm not sure I'm equipped to write the text for this and the text in HTML51 does not seem good enough.
  • Should these elements have rendering rules? They do in HTML51 (strangely enough rbc does too, which otherwise goes unmentioned). Presumably Firefox and Safari implement those. Not sure about Edge.

@Hixie
Copy link
Member

Hixie commented Sep 4, 2015

IMHO keeping ruby sane is a lost cause. We should just spec whatever the implementations implement and not bother trying to fight it. This includes letting authors do whatever the heck it is that the ruby community wants to let them do. I was going to spec what they implemented many months ago, I just never got around to it because of not having editing bandwidth in general.

@sideshowbarker
Copy link
Contributor

@Hixie I’d personally prefer that we at least figure out if/how to include language saying something like:

Documents should not use the rb and rtc elements unnecessarily; if a passage of text in a document can be marked up effectively without rb and rtc, it should be.

(Replace “Documents“ above with “Authors” if we want.)

My rationale is around that is the fact that generally, if authors read the spec and see particular elements in the spec, the authors assume those elements are the “right” thing to use, and they tend to automatically think they should use those elements, and they want to use them.

And in the case of Ruby, they’ll see rb and rtc and say, “I guess because these are here in the spec, I should try to use [the more complex] way of marking up my text with these, rather the [much simpler] way that that doesn’t use them—because otherwise why would the editors put these elements in the spec for me to use?”

So a “Documents should not use the rb and rtc elements unnecessarily” requirement in the spec would help to prevent that—help to prevent authors from wasting their time, and being confused, and asking unnecessary questions in various fora, and giving other authors bad info in various fora, etc.

@annevk
Copy link
Member Author

annevk commented Sep 5, 2015

I emailed folks from Apple https://lists.w3.org/Archives/Public/www-archive/2015Sep/0006.html and Microsoft https://lists.w3.org/Archives/Public/www-archive/2015Sep/0007.html since only Mozilla appears to have anything implemented beyond the parser changes: http://mxr.mozilla.org/mozilla-central/source/layout/style/html.css#791.

@myakura
Copy link

myakura commented Sep 8, 2015

cc. @kojiishi

@annevk
Copy link
Member Author

annevk commented Sep 8, 2015

@nox discovered that the patch for WebKit (see #101 (comment)) is different from HTML51 and from what Chromium and Gecko implemented. In WebKit <rtc><rp> results in <rp> being a sibling rather than a child. Good times.

So based on Microsoft's reply and these findings about WebKit it seems ruby parsing is now three-way forked. Styling is two-way forked with only Mozilla making changes (though not matching the HTML51 rendering section, so three-way forked technically I suppose if you count the specification).

Based on that I'm not entirely sure what to recommend. Everyone resetting to the pre-HTML51-inspired mess seems ideal, but perhaps a good middle ground would be to accept the parsing changes per Chromium and Gecko, and not adopt the styling changes per non-Gecko.

@nox
Copy link
Member

nox commented Sep 8, 2015

This looks like an unintended bug in the WebKit commit:

(WebCore::hasImpliedEndTag): the spec says rb and rtc have implied end tags.
(WebCore::HTMLTreeBuilder::processStartTagForInBody): rb and rtc added. rt excludes rtc from its implied end tags.

Nowhere the message indicates that rp should change behaviour. Never mind, that's why rp did not change behaviour.

nox added a commit to nox/html5ever that referenced this pull request Sep 8, 2015
@domenic
Copy link
Member

domenic commented Sep 8, 2015

I managed to get my hands on an Edge-having computer and confirmed they do not have any special parsing behavior. So regarding the parser, we have four separate behaviors for the four separate rendering engines:

  1. Edge, HTML Standard: no special treatment. rb and rtc map to HTMLUnknownElement.
  2. WebKit, Gecko, W3C HTML 5.1: rb and rtc get implied end tags and map to HTMLElement
  3. WebKit, W3C HTML 5.0: like (2) except rp causes an implied end tag for rtc
  4. Blink: like (2) except rb and rtc map to HTMLUnknownElement

@annevk has been trying to find the reasoning for the change between 5.0 and 5.1 (i.e. between (2) and (3)), without success.

Regarding styling, only Gecko has UA style rules for rb and rtc. So we will probably remove those regardless.

How are we going to achieve compatibility? What should we specify?

To me the front-runners seem to be (1) and (2).

(1) would require Gecko, WebKit and Blink to remove their parser changes. It is attractive in its simplicity.

(2) would require Edge to implement the parser changes, and WebKit to change their parser, and Blink to update their mapping to use HTMLElement.

Can implementers comment? Do either of those two options sound good to you? Tagging in some possibilities: @travisleithead from Edge, @rniwa from WebKit, @kojiishi from Blink, and @hsivonen from Gecko.

@domenic domenic mentioned this pull request Sep 8, 2015
@travisleithead
Copy link
Member

I know Robin Berjon spent some time designing the W3C HTML5.1 model, and since he has better insight into the Ruby community than I, I figured it must be "better". So, I endorse that model ((2) above). The Edge browser has not updated our parser to reflect the W3C HTML5.1 model yet, but we are not opposed to doing so if it helps interoperability.

@rniwa
Copy link

rniwa commented Sep 8, 2015

I don't have a strong opinion for rtc since it's a new element but supporting rb in the parser is very important for compatibility reasons since a lot of Japanese websites do use them.

@kojiishi
Copy link

kojiishi commented Sep 9, 2015

@nox:

This looks like an unintended bug in the WebKit commit:

Sorry slow to catch up, but yes. Actually, we found the spec issue after WebKit patch landed, so Gecko/Blink did in one patch, while WebKit needed two patches.

@kojiishi
Copy link

kojiishi commented Sep 9, 2015

Blink to update their mapping to use HTMLElement.

Elliot already mentioned this is just a bug, I can fix it for Blink.

WebKit, W3C HTML 5.0: like (2) except rp causes an implied end tag for rtc

As @nox indicated in the following comments, my understanding is that WebKit is now interoperable with Gecko.

@nox
Copy link
Member

nox commented Sep 9, 2015

@kojiishi I don't know where I said that they are interoperable. :)

@kojiishi
Copy link

kojiishi commented Sep 9, 2015

@nox:

I don't know where I said that they are interoperable. :)

Sorry, I re-read your comment and, yes, you only said "unintended bug" but didn't say it was fixed later.

The mentioned unintended bug was fixed in this WebKit commit.

@domenic
Copy link
Member

domenic commented Sep 9, 2015

I did just test on my iPad and <rtc><rp> results in a child. So WebKit is in (2) already, good.

It sounds like there is a pretty clear trend toward (2). Edge and Blink are willing to change. So yay, let's do that.

We can deal with styling separately (#121). Assigning back to @annevk to update this PR to use HTMLElement instead of HTMLUnknownElement and remove the styling changes for now.

@domenic
Copy link
Member

domenic commented Sep 9, 2015

nooope, I forgot to test <ruby><rtc><rp>; I just tested <rtc><rp>. Awesome. WebKit does differ and is in a separate (3) category.

@rniwa
Copy link

rniwa commented Sep 9, 2015

We can fix our behavior to match (2) for rtc. Like I said earlier, rtc isn't that important for compatibility as long as rb is supported.

@domenic
Copy link
Member

domenic commented Sep 10, 2015

So this LGTM. However, we do need to add definitions of rb and rtc to the elements section. At the very least we need to say that they use HTMLElement. I will leave it up to you @annevk to decide whether it is OK to merge the parser changes without that. I am not so sure it is though.

@domenic domenic assigned annevk and unassigned domenic Sep 10, 2015
@annevk
Copy link
Member Author

annevk commented Sep 10, 2015

They are defined as obsolete elements. See line 111990. And from line 113398 you can see I made sure rb no longer maps to HTMLUnknownElement.

@domenic
Copy link
Member

domenic commented Sep 10, 2015

Ah OK, I wasn't sure that obsolete elements didn't get their own element definition sections, but upon checking now I see that. Cool. Merging!

@domenic
Copy link
Member

domenic commented Sep 10, 2015

Due to some local git snafus the commit got un-associated with this PR. For posterity, it landed as 8d1c8af.

@kojiishi
Copy link

PR to fix interfaces for rb/rtc in web-platform-tests.

kojiishi added a commit to kojiishi/wpt that referenced this pull request Sep 17, 2015
nox added a commit to nox/html5ever that referenced this pull request Sep 22, 2015
scheib pushed a commit to scheib/chromium that referenced this pull request Sep 23, 2015
…764e9c:

imported csswg-test@2f07c989b36abb566ecfa3a2bf8a2f9192f948d1
imported web-platform-tests@959f1218a7d7c9828cda903c5cd27e1b547431ae

css-writing-modes:
- Added parsing tests in upstream in preparation of unprefixing
  (all fail because we're not unprefixed yet.)
- sideways-left (no support in any UA) was removed from the spec.
  Tests were also removed in upstream, so no longer need to SKIP.
- Fixed descriptions in orthogonal-parent-shrink-to-fit-001*

html/semantics:
- Fixed interfaces.js (see whatwg/html#101)
- 1 new FAIL in pseudo-classes/disabled.html

[email protected],[email protected],[email protected],[email protected],[email protected]
BUG=490511, 492664

Review URL: https://codereview.chromium.org/1358453003

git-svn-id: svn://svn.chromium.org/blink/trunk@202524 bbb929c8-8fbe-4397-9dbb-9b2b20218538
iabudiab added a commit to iabudiab/HTMLKit that referenced this pull request Dec 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

9 participants