Closed Bug 1231485 Opened 9 years ago Closed 8 years ago

Enable layout.css.text-emphasis.enabled by default

Categories

(Core :: Layout, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox46 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 2 obsolete files)

MozReview Request: Bug 1231485 part 1 - Fix text-emphasis shorthand with style part unspecified being computed incorrectly.
58 bytes, text/x-review-board-request
dbaron
: review+
Details
MozReview Request: Bug 1231485 part 2 - Add NeutralChange hint to nsStyleText::MaxDifference().
58 bytes, text/x-review-board-request
dbaron
: review+
Details
MozReview Request: Bug 1231485 part 3 - Fix style tests for text-emphasis properties.
58 bytes, text/x-review-board-request
dbaron
: review+
Details
MozReview Request: Bug 1231485 part 4 - Enable layout.css.text-emphasis.enabled pref.
58 bytes, text/x-review-board-request
dbaron
: review+
Details 
Per discussion with dbaron, it is better to just enable this pref now and remove the pref next week for the next version.
Bug 1231485 - Enable text-emphasis by default.
Attachment #8697071 - Flags: review?(dbaron)
Comment on attachment 8697071 [details]
MozReview Request: Bug 1231485 - Enable text-emphasis by default.

I'm a little worried about bug 1228166 given that it might affect Mongolian, but I suspect we have enough other issues with vertical Mongolian right now that I'm not too worried about shipping this right now.
Attachment #8697071 - Flags: review?(dbaron) → review+
The -moz-use-text-color value makes me confused. It seems to me this value has the same effect as currentColor, but it is handled as a keyword value separately for border-color and outline-color, and for text-decoration-color, it could only be set if no corresponding value is detected in text-decoration shorthand.

I suppose we do not actually need to support this value for text-emphasis-color, and we probably should also remove the special case for this value elsewhere. Probably we can move this keyword to kColorKTable and map it to the same value as currentColor?
Flags: needinfo?(dbaron)
Attachment #8697071 - Attachment is obsolete: true
(In reply to Xidorn Quan [:xidorn] (UTC-5) from comment #6)
> The -moz-use-text-color value makes me confused. It seems to me this value
> has the same effect as currentColor, but it is handled as a keyword value
> separately for border-color and outline-color, and for
> text-decoration-color, it could only be set if no corresponding value is
> detected in text-decoration shorthand.
> 
> I suppose we do not actually need to support this value for
> text-emphasis-color, and we probably should also remove the special case for
> this value elsewhere. Probably we can move this keyword to kColorKTable and
> map it to the same value as currentColor?

So -moz-use-text-color is somewhat different from currentColor in terms of computed values, in our implementation.  The working group keeps changing its mind about which is the right behavior for currentColor.

More specifically, there are two possible ways to handle a specified value of 'currentColor':

 (A) The computed value is the computed value of the 'color' property.

 (B) The computed value is 'currentColor', which is drawn as the computed value of the 'color' property.

The spec for 'currentColor' used to say (A), but now it says (B), but I don't think any implementations have updated (although I could be wrong).  Bug 760345 exists to make this change (see some more details there).  It's probably worth trying, although it may introduce compatibility problems.  And if we do try it, we should push Chromium to change as well, and if they don't, we should revert.

We had -moz-use-text-color predating our implementation of 'currentColor'.  We couldn't switch it to be 'currentColor' for non-inherited properties where it was the initial value, because it would cause the properties containing those style structs to fault out of being stored in the rule tree in the default case.

As far as observable behavior differences go:
 * explicit 'inherit' values behave differently
 * animations work better with (A)
 * text-emphasis works better with (B); text-emphasis is the only color-valued property other than 'color' that inherits

The reason the WG made the change was for text-emphasis-color... so text-emphasis-color definitely needs the (B) behavior.
Flags: needinfo?(dbaron)
(For the purposes of enabling text-emphasis, you should probably stick with the existing -moz-use-text-color value; doing the currentColor fixes should be separate.)
I don't understand what do you mean by "stick with the existing -moz-use-text-color value". Supporting -moz-use-text-color would need some extra code for text-emphasis-color, and I don't see how it could solve anything which only having currentColor would break.

The current implementation of text-emphasis-color uses behavior (A), which breaks some inheritance tests. If I change it to behavior (B), it breaks transitions_events test because it is assumed in the test that the final color value is reported from computed value.
Flags: needinfo?(dbaron)
Oh, so I'd completely forgotten that in bug 1040668 comment #30 I wrote:
> Note that it's a little odd that you're implementing the new
> definition of currentColor for one property when we haven't switched
> any other properties, but that's ok.  (We should probably fix the
> others, though!)

This means the current implementation of text-emphasis-color's currentColor uses behavior (B), unlike all other uses of currentColor in our code, which use behavior (A).  All of our -moz-use-text-color use behavior (B), with code in getComputedStyle to report the used value.

So I guess it's probably OK to stick with that currentColor behavior, but you'll need to adjust test_transitions_per_property (I hope) and (based on your previous comment) test_transitions_events.

But we should probably try switching currentColor at some point soon-ish to make sure the switch is Web-compatible, so that if it's not, we can come up with a new name (other than 'currentColor') for what text-emphasis needs.
Flags: needinfo?(dbaron)
Essentially, the mTextEmphasisColorForeground (which you use for currentColor) does the same thing as the mColumnRuleColorIsForeground / BORDER_COLOR_FOREGROUND that we use for column-rule-color, border-*-color, and text-decoration-color's implementation of -moz-use-text-color.
Looks all fine now.
Attached patch patch (obsolete) — — Splinter Review 
Assignee: nobody → quanxunzhen
Flags: needinfo?(quanxunzhen)

        Attachment #8698299 -
        Flags: review?(dbaron)

    
    

    
  
Comment on attachment 8698299 [details] [diff] [review]
patch

It would probably be good to separate the fixes into a separate patch from the enabling.  (Maybe even more than one patch, but definitely at least one separate patch.)

>diff --git a/layout/style/nsComputedDOMStyle.cpp b/layout/style/nsComputedDOMStyle.cpp
> CSSValue*
> nsComputedDOMStyle::DoGetTextEmphasisColor()
> {
>   nsROCSSPrimitiveValue* val = new nsROCSSPrimitiveValue;
>   const nsStyleText* text = StyleText();
>-  nscolor color = text->mTextEmphasisColorForeground ?
>-    StyleColor()->mColor : text->mTextEmphasisColor;
>-  SetToRGBAColor(val, color);
>+  if (text->mTextEmphasisColorForeground) {
>+    val->SetIdent(eCSSKeyword_currentcolor);
>+  } else {
>+    SetToRGBAColor(val, text->mTextEmphasisColor);
>+  }
>   return val;
> }

I believe this change is wrong.  Even though currentColor is the correct computed value, this should be returning the used value, which should be a color.

Why was this needed?




>diff --git a/layout/style/test/test_transitions_events.html b/layout/style/test/test_transitions_events.html

I don't think these changes should be needed given the above change, although maybe something else would be.

        Attachment #8698299 -
        Flags: review?(dbaron) → review-

    
  

        Attachment #8698299 -
        Attachment is obsolete: true

    
    

    
  
Looking at try push in comment 17, there seems to be nothing wrong related to this anymore.

    
    

    
  
Comment on attachment 8698651 [details]
MozReview Request: Bug 1231485 part 1 - Fix text-emphasis shorthand with style part unspecified being computed incorrectly.

https://reviewboard.mozilla.org/r/28059/#review26101

        Attachment #8698651 -
        Flags: review?(dbaron) → review+

    
    

    
  
Comment on attachment 8698652 [details]
MozReview Request: Bug 1231485 part 2 - Add NeutralChange hint to nsStyleText::MaxDifference().

https://reviewboard.mozilla.org/r/28061/#review26103

        Attachment #8698652 -
        Flags: review?(dbaron) → review+

    
    

    
  
Comment on attachment 8698653 [details]
MozReview Request: Bug 1231485 part 3 - Fix style tests for text-emphasis properties.

https://reviewboard.mozilla.org/r/28063/#review26105

        Attachment #8698653 -
        Flags: review?(dbaron) → review+

    
    

    
  
Comment on attachment 8698654 [details]
MozReview Request: Bug 1231485 part 4 - Enable layout.css.text-emphasis.enabled pref.

https://reviewboard.mozilla.org/r/28065/#review26107

        Attachment #8698654 -
        Flags: review?(dbaron) → review+

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/10d6ef649f6b76cac7eb237753cc2eac20f979ff
Bug 1231485 part 1 - Fix text-emphasis shorthand with style part unspecified being computed incorrectly. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/d32fc26fe99b8a5a5c06c21cb9b8c06f9fd6e870
Bug 1231485 part 2 - Add NeutralChange hint to nsStyleText::MaxDifference(). r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/e04ff2c12fc2b323bee571fcdc562e15598069ff
Bug 1231485 part 3 - Fix style tests for text-emphasis properties. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/171f2e29d31eea6b3daacf329eea2b9f396cd268
Bug 1231485 part 4 - Enable layout.css.text-emphasis.enabled pref. r=dbaron

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: