Closed Bug 744243 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: kennyluck, Assigned: kennyluck)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

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
(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
(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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Layout: Block and Inline → Style System (CSS)
QA Contact: layout.block-and-inline → style-system
Assignee: nobody → kennyluck
Status: NEW → ASSIGNED
Attachment #613837 - Flags: review?(dbaron)
(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 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
Attachment #613837 - Flags: review?(dbaron) → review+
Attached patch comments addressed (obsolete) — Splinter Review
(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).
Attachment #613837 - Attachment is obsolete: true
Keywords: checkin-needed
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.
Priority: -- → P4
Target Milestone: --- → mozilla14
Priority: P4 → P3
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.
Attachment #613891 - Attachment is obsolete: true
Maybe also worth listing that old value in the invalid_values line rather than dropping it entirely?
(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.
Attachment #613949 - Attachment is obsolete: true
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.