556694 - Selection color isn't reverted when input field is specified only background-color

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

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

This is regression of bug 472410. See bug 472410 comment 5 and later.

I'll post the patch.

Comment 1

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

Attachment: Patch v1.0

Comment 2

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

Attachment: Patch v1.1

Should check nsIFrame::IsThemed().

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why do you check mAppearance on one element, and then IsThemed on it and all its ancestors?

I'm a bit concerned about the ancestor check too; something that's a container for arbitrary content can be themed (e.g., toolbox, toolbar, window, dialog, the various vista toolboxes).  Behaving differently if an arbitrary ancestor is themed seems wrong.


Also, I don't understand from the summary what the bug that you're fixing is.  Could you explain?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #3)
> Why do you check mAppearance on one element, and then IsThemed on it and all
> its ancestors?
> 
> I'm a bit concerned about the ancestor check too; something that's a container
> for arbitrary content can be themed (e.g., toolbox, toolbar, window, dialog,
> the various vista toolboxes).  Behaving differently if an arbitrary ancestor is
> themed seems wrong.

Er, sorry, never mind this part; I see what the code is doing now.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Instead of this, I think the changes in bug 472410 to FindNonTransparentBackground are the wrong approach:  FindNonTransparentBackground should take a frame as an argument, walk up the frame tree (using nsLayoutUtils::GetParentOrPlaceholderFor), and return a frame.  This allows FindNonTransparentBackground to also check IsThemed() rather than just mAppearance.  And then you don't need the weird loop to redo the work that FindNonTransparentBackground already did.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #5)
> frame tree (using nsLayoutUtils::GetParentOrPlaceholderFor), and return a

Actually, it should just use nsIFrame::GetParent.  The current behavior of walking the style context tree rather than the frame tree is incorrect for what FindNonTransparentBackground is doing.  That changes what the function does, but for the better.

Comment 7

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

Attachment: Patch v2.0

Thank you. Seems it's better to use nsLayoutUtils::GetParentOrPlaceholderFor().

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 437743 [details] [diff] [review]
Patch v2.0

>diff --git a/layout/base/nsCSSRendering.cpp b/layout/base/nsCSSRendering.cpp
>+    const nsStyleBackground* bg =
>+      frame->GetStyleContext()->GetStyleBackground();

This can be simplified to frame->GetStyleBackground().
>diff --git a/layout/generic/nsTextFrameThebes.cpp b/layout/generic/nsTextFrameThebes.cpp
>+    bgFrame->GetStyleContext()->
>+      GetVisitedDependentColor(eCSSProperty_background_color);

This can just be bgFrame->GetVisitedDependentColor(...).

>diff --git a/layout/tables/nsTableFrame.cpp b/layout/tables/nsTableFrame.cpp
>+  mTableBgColor = bgFrame->GetStyleContext()->GetStyleBackground();

And this can just be bgFrame->GetStyleBackground().

r=dbaron with that

Comment 9

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

Attachment: Patch v2.1

thank you, dbaron!

Comment 10

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

http://hg.mozilla.org/mozilla-central/rev/5a79877003e8