Add reference test for <ol reversed> with display:none children

RESOLVED FIXED in Firefox 52

Status

()

Core
Layout: Misc Code
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: manishearth, Assigned: Petr Gazarov, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla52
good-first-bug
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [lang=html])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

a year ago
In bug 1290813, we fixed `<ol reversed>` so that it gets numbered correctly when descendant `<li>` elements are not direct children.

This inadvertantly fixed another bug. Before this patch, the following code:

<ol reversed>
<li>Four</li>
<li style="display:none">Three</li>
<li>Two</li>
<li>One</li>
</ol>

gets numbered as

4. Four
3. Two
2. One

instead of 

3. Four
2. Two
1. One

(display:none elements aren't counted)

The current Firefox nightly handles this correctly.

We should add a reftest to ensure that this doesn't regress.
(Reporter)

Comment 1

a year ago
If you want to work on this:


I suggest adding one that matches the ones added in https://reviewboard.mozilla.org/r/68254/diff/5#index_header . This can be done with 

`./mach web-platform-test-create --reftest -ref testing/web-platform/tests/html/semantics/grouping-content/the-ol-element/reversed-1-ref.html testing/web-platform/tests/html/semantics/grouping-content/the-ol-element/reversed-1e.html`

and by adding a test at reversed-1e.html with a reversed `<ol>` and a display:none child which renders the same as reversed-1-ref. You can test it with `./mach web-platform-test testing/web-platform/tests/html/semantics/grouping-content/the-ol-element/reversed-1e.html`
(Assignee)

Comment 2

a year ago
I've started to work on this
(Reporter)

Comment 3

a year ago
Thanks! Let me know if you have any questions!
Assignee: nobody → petrgazarov
(Assignee)

Comment 4

a year ago
(In reply to Manish Goregaokar [:manishearth] from comment #3)
> Thanks! Let me know if you have any questions!

Hi Manish,

I'm new to the Mozilla code base and Mercurial workflow (my background is git). I don't know how much time you have to guide me through the problem (would appreciate if you do). Here is one specific question:

I looked at the link you included in the comment about similar tests that you wrote, when I pulled from source however, your changes weren't included, although it says they have already been merged. I could override that by pulling and including the reference (hg pull -r 6eae88b097c45f879597a110f143f8f9fc2aad99 https://reviewboard-hg.mozilla.org/gecko), and then merging. It would be helpful if you could explain 
(or point me to resources) about what the mozilla-mercurial workflow looks like.

Thank you.
Petr
(Reporter)

Comment 5

a year ago
You can use git to work off of http://github.com/mozilla/gecko-dev/ if you wish.

Looks like those tests got removed in bug 1302423 (https://gist.github.com/Manishearth/be7f72f0edf640f453412cd42b2fd1bd for a filtered diff). James, any idea what happened there?

You can still add the new reftest though.
Flags: needinfo?(james)
(Reporter)

Comment 6

a year ago
As far as the workflow is concerned, you just use mercurial or git to make your commits, and use hg export/git format-patch to export patches and upload them here. There are more advanced workflows using mozreview, but we don't need them right now (and you need try-push access to use those).
I think some kind of bad merge must have happened when the tests were upstreamed. Mind re-applying 738847eb38313539c6efc7678a53d84ee9fb0266?
Flags: needinfo?(james)
(Reporter)

Updated

a year ago
Depends on: 1307484
(Reporter)

Comment 9

a year ago
Put an updated/rebased patch in https://reviewboard.mozilla.org/r/83300/diff/2#index_header . There are pull/import links on the side if you need. This patch isn't necessary for your work though, you just need to add tests like those.
(Reporter)

Comment 10

a year ago
Any updates?
Flags: needinfo?(petrgazarov)
(Assignee)

Comment 11

a year ago
(In reply to Manish Goregaokar [:manishearth] from comment #10)
> Any updates?

I'm sorry haven't had time last week due to being busy at work, but will look this week. Thank you for all of the info.
(Reporter)

Comment 12

a year ago
Okay, thanks
Flags: needinfo?(petrgazarov)
(Assignee)

Comment 13

a year ago
Hey Manish,

I've been able to set up git workflow, however it looks like I don't have enough permissions to push to try server nor mozilla-central. Can you please clarify what you mean by "Put an updated/rebased patch in https://reviewboard.mozilla.org/r/83300/diff/2#index_header"? I've tried the import/pull commands on the right hand panel, however they don't work because my version control system is git and not hg. Thank you and apologies in advance if this question is too basic.
(Reporter)

Comment 14

a year ago
Ignore that, the tests landed on master. Just pull the latest mozilla-central code (from cinnabar or gecko-dev),  and add the extra test.
(Assignee)

Comment 15

a year ago
Created attachment 8801540 [details] [diff] [review]
Patch Fix-bug-1300944-Add-reference-test-for-ol-reversed-w.patch
(Reporter)

Comment 16

a year ago
Comment on attachment 8801540 [details] [diff] [review]
Patch Fix-bug-1300944-Add-reference-test-for-ol-reversed-w.patch

Review of attachment 8801540 [details] [diff] [review]:
-----------------------------------------------------------------

This test won't pass -- are you running it with `./mach web-platform-test /path/to/test`?

::: testing/web-platform/tests/html/semantics/grouping-content/the-ol-element/reversed-1e.html
@@ +3,5 @@
> +<title>Reverse numbering should not count display:none elements</title>
> +<link rel=match href="reversed-1-ref.html">
> +<link rel=help href="https://html.spec.whatwg.org/#attr-ol-reversed">
> +<ol reversed>
> +  <li>Four</li>

I think this should be "Three"

We need it to look exactly like the reversed-ref file when rendered.
Attachment #8801540 - Flags: review-
(Assignee)

Comment 17

a year ago
Created attachment 8801552 [details] [diff] [review]
0002-Fix-bug-1300944-Add-reference-test-for-ol-reversed-w.patch

Hi, thanks for the quick review and sorry about the first patch. I'm still setting up all the dependencies locally, but I think I got everything in a good state now. The test passes with the `mach web-platform-tests testing/web-platform/tests/html/semantics/grouping-content/the-ol-element/reversed-1e.html` command.
(Reporter)

Updated

a year ago
Attachment #8801540 - Attachment is obsolete: true
(Reporter)

Comment 18

a year ago
Comment on attachment 8801552 [details] [diff] [review]
0002-Fix-bug-1300944-Add-reference-test-for-ol-reversed-w.patch

Review of attachment 8801552 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good! I myself can't grant review on this, but Xidorn can.

Additionally, the commit message should be of the format `Bug 1300944 - blah blah blah; r?xidorn`. I can amend this for you once it gets reviewed if you want, though, just keep it in mind for future patches. If fixing up the patch, be sure to re-request review.

I pushed to try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e103b1af1a10c543b13ebbc0f768ced5940f0765
Attachment #8801552 - Flags: review?(xidorn+moz)
Attachment #8801552 - Flags: feedback+
(Reporter)

Updated

a year ago
Status: NEW → ASSIGNED
Comment on attachment 8801552 [details] [diff] [review]
0002-Fix-bug-1300944-Add-reference-test-for-ol-reversed-w.patch

Review of attachment 8801552 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly looks good to me. Please fix the issue below, and change the commit message as adviced in comment 18. I'd like to check the updated patch.

::: testing/web-platform/tests/html/semantics/grouping-content/the-ol-element/reversed-1e.html
@@ +7,5 @@
> +  <li>Three</li>
> +  <li style="display:none">Three</li>
> +  <li>Two</li>
> +  <li>One</li>
> +  </div>

Looks like this </div> is useless. You should remove it.
Attachment #8801552 - Flags: review?(xidorn+moz)
Note that this actually doesn't match what the spec says at this moment. The spec doesn't mention that the starting value should depend on rendered <li> only. But given that all browsers now agree on this behavior, and it does make sense, I believe the spec would eventually be updated accordingly.

I've raised an issue to HTML spec for this: https://github.com/whatwg/html/issues/1911
(Reporter)

Comment 21

a year ago
Any updates? Only a few minor adjustments to the patch and then we can land it.
Flags: needinfo?(petrgazarov)
(Assignee)

Comment 22

a year ago
Created attachment 8803247 [details] [diff] [review]
0003-Bug-1300944-Add-reference-test-for-ol-reversed-with-.patch

Thank you for your timely followups and sorry for taking so long with this!
(Reporter)

Updated

a year ago
Attachment #8801552 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8803247 - Flags: review?(xidorn+moz)
Attachment #8803247 - Flags: feedback+
Attachment #8803247 - Flags: review?(xidorn+moz) → review+
Keywords: checkin-needed

Comment 23

a year ago
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9533a537893
Add reference test for <ol reversed> with display:none children; r=xidorn
Keywords: checkin-needed

Comment 25

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c9533a537893
https://hg.mozilla.org/mozilla-central/rev/3a6f7a62d77f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Comment 26

a year ago
Manish, just following up. Anything left to do here?
(Reporter)

Comment 27

a year ago
Nah, it's fixed. Thanks!
Flags: needinfo?(petrgazarov)
You need to log in before you can comment on or make changes to this bug.