Last Comment Bug 744243 - 'border-image' shorthand should not accept odd trailing slash
: 'border-image' shorthand should not accept odd trailing slash
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: All All
: P3 trivial (vote)
: mozilla14
Assigned To: Kang-Hao (Kenny) Lu [:kennyluck]
:
:
Mentors:
http://lists.w3.org/Archives/Public/w...
Depends on:
Blocks: 497995
  Show dependency treegraph
 
Reported: 2012-04-10 15:59 PDT by Kang-Hao (Kenny) Lu [:kennyluck]
Modified: 2012-05-07 09:33 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
small patch with a Mochitest included (3.38 KB, patch)
2012-04-10 18:01 PDT, Kang-Hao (Kenny) Lu [:kennyluck]
dbaron: review+
Details | Diff | Splinter Review
comments addressed (2.33 KB, patch)
2012-04-11 00:17 PDT, Kang-Hao (Kenny) Lu [:kennyluck]
no flags Details | Diff | Splinter Review
corrected a test and fixed a typo. (2.75 KB, patch)
2012-04-11 04:50 PDT, Kang-Hao (Kenny) Lu [:kennyluck]
no flags Details | Diff | Splinter Review
add the deleted test to "invalid_values" (2.87 KB, patch)
2012-04-11 21:40 PDT, Kang-Hao (Kenny) Lu [:kennyluck]
no flags Details | Diff | Splinter Review

Description Kang-Hao (Kenny) Lu [:kennyluck] 2012-04-10 15:59:53 PDT
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] 2012-04-10 16:01:57 PDT
(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-7 (busy September 14-25) 2012-04-10 16:22:07 PDT
(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] 2012-04-10 18:01:05 PDT
Created attachment 613837 [details] [diff] [review]
small patch with a Mochitest included
Comment 4 Kang-Hao (Kenny) Lu [:kennyluck] 2012-04-10 18:06:02 PDT
(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-7 (busy September 14-25) 2012-04-10 23:07:46 PDT
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] 2012-04-11 00:17:20 PDT
Created attachment 613891 [details] [diff] [review]
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-7 (busy September 14-25) 2012-04-11 00:33:47 PDT
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] 2012-04-11 04:50:02 PDT
Created attachment 613949 [details] [diff] [review]
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-7 (busy September 14-25) 2012-04-11 12:46:01 PDT
Maybe also worth listing that old value in the invalid_values line rather than dropping it entirely?
Comment 10 Kang-Hao (Kenny) Lu [:kennyluck] 2012-04-11 21:40:19 PDT
Created attachment 614275 [details] [diff] [review]
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 12 :Ms2ger (⌚ UTC+1/+2) 2012-04-12 10:18:33 PDT
https://hg.mozilla.org/mozilla-central/rev/987f9a47cee9
Comment 13 Eric Shepherd [:sheppy] 2012-05-07 09:33:34 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.