Improve serialization of 'border-radius'

RESOLVED FIXED in mozilla25

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sebo, Assigned: poiru)

Tracking

Trunk
mozilla25
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=dbaron])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 768010 [details]
Test case for 'border-radius' serialization

When accessing the borderRadius property inside the cssRules of a style sheet the value is not serialized as you would expect.

E.g. when you define "border-radius: 5px;" in your CSS, the value will be expanded to "5px 5px 5px 5px" when you read it.

See the attached test case for some more clarification.

Sebastian
The code for this is in Declaration::GetValue in layout/style/Declaration.cpp; should be a relatively straightforward patch.
Whiteboard: [mentor=dbaron]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #768010 - Attachment description: Test case for 'border-radius' not getting serialized → Test case for 'border-radius' serialization
Attachment #768010 - Attachment mime type: text/plain → text/html
(Reporter)

Comment 2

4 years ago
Because I just saw the code for this right now, the same counts for the value of the -moz-outline-radius, of course.

Sebastian
(Assignee)

Comment 3

4 years ago
Created attachment 769409 [details] [diff] [review]
Part 1: Improve Declaration::GetValue for 'border-radius'
Attachment #769409 - Flags: review?(dbaron)
(Assignee)

Comment 4

4 years ago
Created attachment 769410 [details] [diff] [review]
Part 2: Refactor Declaration::GetValue for 'margin' and friends

Hi. I've separated the patch into two parts. Part 1 fixes the bug and Part 2 reduces code duplication.
Attachment #769410 - Flags: review?(dbaron)
Comment on attachment 769409 [details] [diff] [review]
Part 1: Improve Declaration::GetValue for 'border-radius'

Thanks for the patch -- this largely looks good, though I have a bunch of comments, many of which are substantially about local style.

># HG changeset patch
># Parent cbb24a4a96afda6d2e765536b284b281c122a60f
># User Birunthan Mohanathas <birunthan@mohanathas.com>
>Bug 887502 - Part 1: Improve Declaration::GetValue for 'border-radius'
>Shorthand notation is now used for 'border-radius' when possible.

I think the first line isn't actually helpful; it's the second line that's important.  I'd suggest a commit message like:

Bug 887502 Part 1: Coalesce corners rather than repeating when serializing specified values of 'border-radius'.

(The first line of a commit message is semantically different from the remaining lines; only the first line is shown in a short view; in other views the full message is shown.  So the first line on its own should be a complete summary, with more details below; see, for example, https://hg.mozilla.org/mozilla-central/rev/830111e10951 .)

>diff --git a/layout/style/Declaration.cpp b/layout/style/Declaration.cpp
>--- a/layout/style/Declaration.cpp
>+++ b/layout/style/Declaration.cpp
>@@ -102,16 +102,53 @@ Declaration::AppendValueToString(nsCSSPr
>   if (!val) {
>     return false;
>   }
> 
>   val->AppendToString(aProperty, aResult);
>   return true;
> }
> 
>+namespace {

I think we actually tend to stay away from anonymous namespace because they don't get along well with debuggers. Could you just declare the function static instead?

>+
>+// Helper to append |aString| with the shorthand notation of CSS properties
>+// with the syntax "[ <length> | <percentage> ]{1,4}" (e.g. padding).
>+// |aProperties| and |aValues| are expected to have 4 elements.

I don't think this is at all specific to <length> or <percentage>; it simply works for any shorthand that uses the sides/corners coalescing pattern.

>+void
>+AppendOneToFourShorthandValuesToString(const nsCSSProperty aProperties[],
>+                                       const nsCSSValue* aValues[],
>+                                       nsAString& aString)

Rather than "OneToFourShorthandValues" I'd use "SidesShorthand" since this is a pattern for shorthands that are for sides or corners.

>@@ -235,38 +272,40 @@ Declaration::GetValue(nsCSSProperty aPro
>         data->ValueFor(subprops[1]),
>         data->ValueFor(subprops[2]),
>         data->ValueFor(subprops[3])
>       };
> 
>       // For compatibility, only write a slash and the y-values
>       // if they're not identical to the x-values.
>       bool needY = false;
>+      const nsCSSValue* xVals[4] = {};

drop the " = {}"

>       for (int i = 0; i < 4; i++) {
>         if (vals[i]->GetUnit() == eCSSUnit_Pair) {
>           needY = true;
>-          vals[i]->GetPairValue().mXValue.AppendToString(subprops[i], aValue);
>+          xVals[i] = &vals[i]->GetPairValue().mXValue;
>         } else {
>-          vals[i]->AppendToString(subprops[i], aValue);
>+          xVals[i] = vals[i];
>         }
>-        if (i < 3)
>-          aValue.Append(PRUnichar(' '));
>       }
> 
>+      AppendOneToFourShorthandValuesToString(subprops, xVals, aValue);
>+
>       if (needY) {
>         aValue.AppendLiteral(" / ");
>+        const nsCSSValue* yVals[4] = {};
>         for (int i = 0; i < 4; i++) {
>           if (vals[i]->GetUnit() == eCSSUnit_Pair) {
>-            vals[i]->GetPairValue().mYValue.AppendToString(subprops[i], aValue);
>+            yVals[i] = &vals[i]->GetPairValue().mYValue;
>           } else {
>-            vals[i]->AppendToString(subprops[i], aValue);
>+            yVals[i] = vals[i];
>           }
>-          if (i < 3)
>-            aValue.Append(PRUnichar(' '));
>         }
>+
>+        AppendOneToFourShorthandValuesToString(subprops, yVals, aValue);

While you're doing this, I think it makes sense to merge the accumulation of yVals into the same loop as xVals rather than have two separate loops.  (You still need |needY|.)


r=dbaron with those changes, but I'd like to have a look at the revised patch, so marking as review-
Attachment #769409 - Flags: review?(dbaron) → review-
Comment on attachment 769410 [details] [diff] [review]
Part 2: Refactor Declaration::GetValue for 'margin' and friends

r=dbaron when adjusted to account for the rename in the previous patch

You'll want to upload a revised patch with "r=dbaron" included (and the revision) so that you can use checkin-needed per
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in
(that's the only piece missing)
Attachment #769410 - Flags: review?(dbaron) → review+
(Assignee)

Comment 7

4 years ago
Created attachment 769419 [details] [diff] [review]
Part 1: Coalesce corners rather than repeating when serializing specified values of 'border-radius'

Thanks for the comments. I've updated the patch to address them.
Attachment #769409 - Attachment is obsolete: true
Attachment #769419 - Flags: review?(dbaron)
(Assignee)

Comment 8

4 years ago
Created attachment 769420 [details] [diff] [review]
Part 2: Refactor Declaration::GetValue for 'margin' and friends
Attachment #769410 - Attachment is obsolete: true
Attachment #769420 - Flags: review+
Comment on attachment 769419 [details] [diff] [review]
Part 1: Coalesce corners rather than repeating when serializing specified values of 'border-radius'

r=dbaron
Attachment #769419 - Flags: review?(dbaron) → review+
(If you attach a version of part 1 with the r=dbaron, then you can add the checkin-needed keyword, and somebody should then land it within a day or so.  Though maybe more due to holidays this week...)
(Assignee)

Comment 11

4 years ago
Created attachment 769464 [details] [diff] [review]
Part 1: Coalesce corners rather than repeating when serializing specified values of 'border-radius'

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #10)
> (If you attach a version of part 1 with the r=dbaron, then you can add the
> checkin-needed keyword, and somebody should then land it within a day or so.
> Though maybe more due to holidays this week...)

Can you assign me so that I can add checkin-needed?
Attachment #769419 - Attachment is obsolete: true
Attachment #769464 - Flags: review+
Assignee: nobody → birunthan
Keywords: checkin-needed
(In reply to Birunthan Mohanathas [:poiru] from comment #11)
> Can you assign me so that I can add checkin-needed?

I also gave you canconfirm and editbugs so you can do this yourself in general.  You should read https://developer.mozilla.org/en-US/docs/What_to_do_and_what_not_to_do_in_Bugzilla
One other note (for the future; no need to correct anything here):  the way this was split into two patches was a little bit odd.  I think it would have been somewhat clearer to swap the order of the two patches, except that the new function would still be introduced in the new part 1.  This would make part 1 a refactoring patch that moves code from inside of GetValue into its own function, and then part 2 would change the behavior for border-radius by using that code.
(Assignee)

Comment 14

4 years ago
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #12)
> I also gave you canconfirm and editbugs so you can do this yourself in
> general.  You should read
> https://developer.mozilla.org/en-US/docs/
> What_to_do_and_what_not_to_do_in_Bugzilla

Great, thank you.

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #13)
> One other note (for the future; no need to correct anything here):  the way
> this was split into two patches was a little bit odd.  I think it would have
> been somewhat clearer to swap the order of the two patches, except that the
> new function would still be introduced in the new part 1.  This would make
> part 1 a refactoring patch that moves code from inside of GetValue into its
> own function, and then part 2 would change the behavior for border-radius by
> using that code.

Agreed. I'll keep it in mind for the future.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5729545971af
https://hg.mozilla.org/integration/mozilla-inbound/rev/0047417b0635
Flags: in-testsuite+
Keywords: checkin-needed
Backed out for test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ec0cda67e41

https://tbpl.mozilla.org/php/getParsedLog.php?id=24785229&tree=Mozilla-Inbound
(Assignee)

Comment 17

4 years ago
Created attachment 769746 [details] [diff] [review]
Part 1: Coalesce corners rather than repeating when serializing specified values of 'border-radius'

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> Backed out for test failures.

Fixed now. Sorry for the hassle!
Attachment #769464 - Attachment is obsolete: true
Attachment #769746 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Attachment #769746 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f709b70c5ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8d4f183cabf
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0f709b70c5ef
https://hg.mozilla.org/mozilla-central/rev/a8d4f183cabf
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Reporter)

Comment 20

4 years ago
I think I never saw an issue I reported being fixed that fast. Thanks for that!

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