877857 - With overlapped overlay scrollbars on 10.8, horizontal scrollbar should appear above vertical on hover

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[David Corry]

Attachment: Firefox vs Finder

Small cosmetic problem with bug 873012 - when you hover over the horizontal scrollbar, it appears behind the vertical one, when it should appear above.

Attachment shows Finder's behavior vs. Firefox's.

Comment 1

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 778892 [details] [diff] [review]
Patch

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

Where does this hover attribute come from?

Comment 3

[Stephen A Pohl [:spohl]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> 
> Where does this hover attribute come from?

This is coming from ScrollbarActivity::HandleEventForScrollbar.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 778892 [details] [diff] [review]
Patch

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2039,5 @@
>    } else {
> +    if (appendToTop) {
> +      aDest->AppendToTop(aSource);
> +    } else {
> +      aDest->AppendToBottom(aSource);

This is going to potentially put scrollbars below content, because AppendToBottom will append to the bottom of the destination list, which includes content.

One way to fix this would be to have nsGfxScrollFrameInner::AppendScrollPartsTo collect the child frames into a list (an nsAutoTArray<nsIFrame*,3>), and then sort that list so that the frames with the hover attribute are at the end of the list, then loop over those child frames so the frames with the hover attribute are processed last.

Comment 5

[Stephen A Pohl [:spohl]]

Attachment: bug877857

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 779221 [details] [diff] [review]
bug877857

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2042,5 @@
> +  }
> +  bool LessThan(nsIFrame* A, nsIFrame* B) const {
> +    bool aHovered = nsNativeTheme::CheckBooleanAttr(A, nsGkAtoms::hover);
> +    bool bHovered = nsNativeTheme::CheckBooleanAttr(B, nsGkAtoms::hover);
> +    return !aHovered && bHovered;

Don't pull in nsNativeTheme::CheckBooleanAttr for this.

All we need to do here is check frame->GetContent()->HasAttr(kNameSpaceID_None, nsGkAtoms::hover). You can move that to a local static helper that takes an nsIFrame* if you want.

Comment 7

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 8

[Stephen A Pohl [:spohl]]

ttps://hg.mozilla.org/integration/mozilla-inbound/rev/1a4da037387a

Comment 9

[Stephen A Pohl [:spohl]]

That should have been:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a4da037387a

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1a4da037387a