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

Sync with Blink #67

Closed
wants to merge 3 commits into from
Closed

Conversation

gsnedders
Copy link
Member

This takes us up to c62ef30ff36a05ef35a7c9eb7fb81fe76c5c2b9c in the Blink repo.

Few conflicts, and I think I've handled them correctly…

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/5781

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@nox
Copy link
Member

nox commented Sep 9, 2015

The following test case looks incorrect:

<b><em><dcell><postfield><postfield><postfield><postfield><missing_glyph><missing_glyph><missing_glyph><missing_glyph><hkern><aside></b></em>

The em element shouldn't be considered as an active formatting element around the aside element. I think there is a bug in Blink's implementation of adoption agency algorithm.

The spec says:

  1. Let node and last node be furthest block. Follow these steps:
    1. Let inner loop counter be zero.
    2. Inner loop: Increment inner loop counter by one.
      (…)
    3. If inner loop counter is greater than three and node is in the list of active formatting elements, then remove node from the list of active formatting elements.

But the code says:

        // 9.1, 9.2, 9.3 and 9.11 are covered by the for() loop.
        for (int i = 0; i < innerIterationLimit; ++i) {

The variable i is incremented at the end of each iteration in the code, while the spec mandates it to do it at its start. Isn't that the origin of the discrepancy between Blink and the rest of the world?

@nox
Copy link
Member

nox commented Sep 9, 2015

Proof-reading my post, I realise I was just trying to see why the rest of the world behave differently between the two following cases:

<b><em><foo><foo><aside></b></em>
<b><em><foo><foo><foo><aside></b></em>

I realise now that it's not the test case I mentioned, but I still think that inner loop in Blink's code is the culprit. Why does it even stop after 3 iterations? Why is it labeled step 9 instead of 13? :(

@gsnedders
Copy link
Member Author

It's equivalent to the Blink TC, so it's fine. I want to check with those who wrote it whether we can reduce the TC down to that. (And probably add the two-element case to make sure that works.)

@gsnedders
Copy link
Member Author

If it were a simple off-by-one error in the Blink code, <b><em><foo><foo><foo><foo><foo><foo><foo><foo><foo><foo><aside></b></em> would be interoperable, but it isn't. There's some deeper, underlying different here. sighs

@gsnedders
Copy link
Member Author

Blink are entirely missing:

If inner loop counter is greater than three and node is in the list of active formatting elements, then remove node from the list of active formatting elements.

OK, so Gecko is definitely right.

This takes us up to c62ef30ff36a05ef35a7c9eb7fb81fe76c5c2b9c in the Blink repo.

Conflicts:
	tree-construction/adoption01.dat
	tree-construction/domjs-unsafe.dat
	tree-construction/template.dat
	tree-construction/tests11.dat
	tree-construction/tests19.dat
@gsnedders gsnedders force-pushed the blink_updates_20150902 branch from 88ec8b8 to 149c988 Compare September 16, 2015 01:59
@gsnedders
Copy link
Member Author

And landed…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants