Closed Bug 1300944 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: manishearth, Assigned: petrgazarov, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=html])

Attachments

(1 file, 2 obsolete files)

0003-Bug-1300944-Add-reference-test-for-ol-reversed-with-.patch
2.23 KB, patch
xidorn
: review+
manishearth
: feedback+
Details | Diff | Splinter Review 
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.
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`
I've started to work on this
Thanks! Let me know if you have any questions!
Assignee: nobody → petrgazarov
(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
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)
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)
Depends on: 1307484
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.
Any updates?
Flags: needinfo?(petrgazarov)
(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.
Okay, thanks
Flags: needinfo?(petrgazarov)
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.
Ignore that, the tests landed on master. Just pull the latest mozilla-central code (from cinnabar or gecko-dev),  and add the extra test.
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-
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.
Attachment #8801540 - Attachment is obsolete: true
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+
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
See Also: → https://github.com/whatwg/html/issues/1911
Any updates? Only a few minor adjustments to the patch and then we can land it.
Flags: needinfo?(petrgazarov)
Thank you for your timely followups and sorry for taking so long with this!
Attachment #8801552 - Attachment is obsolete: true
Attachment #8803247 - Flags: review?(xidorn+moz)
Attachment #8803247 - Flags: feedback+
Attachment #8803247 - Flags: review?(xidorn+moz) → review+
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
https://hg.mozilla.org/mozilla-central/rev/c9533a537893
https://hg.mozilla.org/mozilla-central/rev/3a6f7a62d77f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Manish, just following up. Anything left to do here?
Nah, it's fixed. Thanks!
Flags: needinfo?(petrgazarov)
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: