744243 - 'border-image' shorthand should not accept odd trailing slash

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Kang-Hao (Kenny) Lu [:kennyluck]]

Gecko now accepts an odd trailing slash for the 'border-image-slice'/'border-image-width'/'border-image-width' part, following an older version of the CSS3 B&B spec. I raised that as an issue on www-style[1] and the editors decided on[2] Option C (disallow trailing slash), not my favorite option (which is D) but still falls into what I consider acceptable. I won't be very surprised if we end up having Option D in the final version of the spec though since that's what's implemented in WebKit.

However, I'll just submit a patch with mochitest implementing Option C in a short time. It shouldn't take more than an hour.

ps. This is relevant to bug 497995 but I don't have access to the bug metadata. May I get the access by the way?

[1] http://lists.w3.org/Archives/Public/www-style/2012Mar/0179
[2] http://lists.w3.org/Archives/Public/www-style/2012Mar/0605

Comment 1

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to Kang-Hao (Kenny) Lu from comment #0)
> ps. This is relevant to bug 497995 but I don't have access to the bug
> metadata. May I get the access by the way?

Hmm... It seems that I can do so as the reporter, but still want the access :p

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Kang-Hao (Kenny) Lu from comment #0)
> ps. This is relevant to bug 497995 but I don't have access to the bug
> metadata. May I get the access by the way?

I gave you canconfirm and editbugs.   See https://developer.mozilla.org/en/What_to_do_and_what_not_to_do_in_Bugzilla

Comment 3

[Kang-Hao (Kenny) Lu [:kennyluck]]

Attachment: small patch with a Mochitest included

Comment 4

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to David Baron [:dbaron] from comment #2)
> (In reply to Kang-Hao (Kenny) Lu from comment #0)
> > ps. This is relevant to bug 497995 but I don't have access to the bug
> > metadata. May I get the access by the way?
> 
> I gave you canconfirm and editbugs.   See
> https://developer.mozilla.org/en/What_to_do_and_what_not_to_do_in_Bugzilla

Great! Thanks. Reading it now.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 613837 [details] [diff] [review]
small patch with a Mochitest included

There's no need to add an entire new mochitest file to test this.

So instead of adding test_bug744243.html and modifying the Makefile.in, just add your two values to the invalid_values line for border-image in property_database.js, and check that test_property_syntax_errors.html shows failures without your patch.  (It might be worth adding a few other related tests, too.)

r=dbaron with that changed

Comment 6

[Kang-Hao (Kenny) Lu [:kennyluck]]

Attachment: comments addressed

(In reply to David Baron [:dbaron] from comment #5)
> Comment on attachment 613837 [details] [diff] [review]
> small patch with a Mochitest included
> 
> There's no need to add an entire new mochitest file to test this.
> 
> So instead of adding test_bug744243.html and modifying the Makefile.in, just
> add your two values to the invalid_values line for border-image in
> property_database.js, and check that test_property_syntax_errors.html shows
> failures without your patch.  (It might be worth adding a few other related
> tests, too.)

Done. I added two more tests (two trailing slashes, no number before slash).

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/986091dac358

I made two changes:
 * tweaked the checkin comment to use the property name rather than the function it's parsed in
 * fixed a typo (intial -> initial)

Thanks for fixing this.

Comment 8

[Kang-Hao (Kenny) Lu [:kennyluck]]

Attachment: corrected a test and fixed a typo.

ms2ger mentioned that the patch is backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/46eacf64298a , and I noticed that "other_values" had an invalid value with trailing slash. I didn't realize that and didn't run the whole mochitest suite. Sorry :(

I also made a typo in the commit message "744234" should read "744243". These are now corrected.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Maybe also worth listing that old value in the invalid_values line rather than dropping it entirely?

Comment 10

[Kang-Hao (Kenny) Lu [:kennyluck]]

Attachment: add the deleted test to "invalid_values"

(In reply to David Baron [:dbaron] from comment #9)
> Maybe also worth listing that old value in the invalid_values line rather
> than dropping it entirely?

I reason I didn't add it ("url('border.png') 27 27 27 27 /") to "invalid_values" back that is because I wrote a test ("url('border.png') 1 /") independently which pretty much tests the same thing.

Anyway, I add "url('border.png') 27 27 27 27 /" to "invalid_values" now. I'm adding "checkin-needed" again.

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/987f9a47cee9

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/987f9a47cee9

Comment 13

[Eric Shepherd [:sheppy]]

If I interpret the doc here correctly:

https://developer.mozilla.org/En/CSS/border-image

This looks like our doc is for the new behavior to me.

Mentioned on Firefox 14 for developers.