Closed Bug 877857 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: bugmail, Assigned: spohl)

References

Details

(Whiteboard: [lion-scrollbars-])

Attachments

(2 files, 2 obsolete files)

Attached image 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.
Blocks: 873012
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [lion-scrollbars-]
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Attachment #778892 - Flags: review?(roc)
Comment on attachment 778892 [details] [diff] [review]
Patch

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

Where does this hover attribute come from?
(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 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.
Attached patch bug877857 (obsolete) — Splinter Review
Attachment #778892 - Attachment is obsolete: true
Attachment #778892 - Flags: review?(roc)
Attachment #779221 - Flags: review?(roc)
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.
Attached patch PatchSplinter Review
Attachment #779221 - Attachment is obsolete: true
Attachment #779221 - Flags: review?(roc)
Attachment #779579 - Flags: review?(roc)
ttps://hg.mozilla.org/integration/mozilla-inbound/rev/1a4da037387a
https://hg.mozilla.org/mozilla-central/rev/1a4da037387a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.