1445599 - GetTemporaryStyleForFocusedPositionedElement is broken.

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

I was looking at removing editor calls to GetPropertyCSSValue when I realized that this one is actually unreachable since bug 1339394.

I was going to rewrite it to use nsStyleContext or what not, but we may as well remove it.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Remove HTMLEditor::GetTemporaryStyleForFocusedPositionedElement

Comment 2

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> Created attachment 8958773 [details] [diff] [review]
> Remove HTMLEditor::GetTemporaryStyleForFocusedPositionedElement

The commit message should also say something like:

    The background color won't serialize to transparent since bug 1339394 (FF 54).
    
    Since nobody has complained about this regression, it seems unlikely this is a
    useful feature. I can rewrite it to not use GetPropertyCSSValue instead, but
    nixing it is easier.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8958773 [details] [diff] [review]
Remove HTMLEditor::GetTemporaryStyleForFocusedPositionedElement

Review of attachment 8958773 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/HTMLAbsPositionEditor.cpp
@@ -618,5 @@
> -  rv =
> -    CSSEditUtils::GetComputedProperty(aElement, *nsGkAtoms::backgroundColor,
> -                                      bgColorStr);
> -  NS_ENSURE_SUCCESS(rv, rv);
> -  if (!bgColorStr.EqualsLiteral("transparent")) {

I still don't understand the reason why you're trying to remove the feature yet.

Do you mean that this is always true, so, this method never returns "black" and "white" anymore?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Right, transparent serializes as rgba(0, 0, 0, 0) since the bug mentioned in comment 2.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> Right, transparent serializes as rgba(0, 0, 0, 0) since the bug mentioned in
> comment 2.

Well, then, shouldn't you just change the check from "transparent" to "rgba(0, 0, 0, 0)"? This is a code to support absolute positioned boxes. That must be really rare case to edit it since most HTML editors in the real world is just rich text editors. So, it's not safe to remove the feature itself by the reason "no bugs are reported".

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Sure, I can do that if needed. I wanted to get rid of GetPropertyCSSValue and this was effectively dead code. If you think it's worth resurrecting the feature I can rewrite it to use other APIs indeed.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: With comments.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Unbreak HTMLEditor::GetTemporaryStyleForFocusedPositionedElement

Whoops, the other patch should belong to other bug. This one belongs to this bug.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8959108 [details] [diff] [review]
Unbreak HTMLEditor::GetTemporaryStyleForFocusedPositionedElement

>-  if (r >= BLACK_BG_RGB_TRIGGER &&
>-      g >= BLACK_BG_RGB_TRIGGER &&
>-      b >= BLACK_BG_RGB_TRIGGER) {
>+  if (NS_GET_R(color) >= kBlackBgTrigger ||
>+      NS_GET_G(color) >= kBlackBgTrigger ||
>+      NS_GET_B(color) >= kBlackBgTrigger) {
>     aReturn.AssignLiteral("black");
>   } else {

Looks like you changed && to ||. It seems that it's not your intention. If so, please fix them before landing.  Otherwise, looks good.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b96b1d081cdf
Unbreak HTMLEditor::GetTemporaryStyleForFocusedPositionedElement, and make it stop using GetPropertyCSSValue. r=masayuki

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/b96b1d081cdf