Closed Bug 343097 Opened 15 years ago Closed 15 years ago

Cleaner arrowscrollbox-clicktoscroll binding

Categories

(Toolkit :: XUL Widgets, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8.1beta2

People

(Reporter: mano, Assigned: mano)

References

Details

(Keywords: fixed1.8.1)

Attachments

(5 files, 2 obsolete files)

Bug 318168 added handleOnClick and handleOnCommand to the arrowscrollbox binding API (for internal use, called in each anonymouse node oncommand/onclick attributes), we can avoid this by adding XBL event handlers.
Taking.
Assignee: nobody → bugs.mano
Target Milestone: --- → mozilla1.8.1beta2
What I really don't understand is why do we have both onclick and oncommand handlers. AFAICT the only way to get a command event for these buttons is via a click event (i.e. the click event is the sourceEvent for the command event), this tells me we scroll twice - once by pixels, once by "lines".
Priority: -- → P1
Morphing this a bit to correspond my tree ;)
Summary: arrowscrollbox should use XBL <handler>s instead of exposing handleOnClick and handleOnCommand in its API → Cleaner arrowscrollbox-clicktoscroll binding
Attached patch patch (obsolete) — Splinter Review
Attachment #227528 - Flags: second-review?(mconnor)
Attachment #227528 - Flags: first-review?(sspitzer)
Note to self: also need to fix the drag handler in tabbrowser.
Attached patch patchSplinter Review
Attachment #227541 - Flags: second-review?(mconnor)
Attachment #227541 - Flags: first-review?(sspitzer)
Attachment #227528 - Attachment is obsolete: true
Attachment #227528 - Flags: second-review?(mconnor)
Attachment #227528 - Flags: first-review?(sspitzer)
Comment on attachment 227541 [details] [diff] [review]
patch

r=sspitzer, this is much better and cleaner than how I did it originally.
Attachment #227541 - Flags: first-review?(sspitzer) → first-review+
Flags: blocking-firefox2?
Whiteboard: [have patch]
Would it be possible to make holding down the button scroll continuously (something like hovering a normal arrowscrollbox does)? Clicking manically on those things is going to give us all RSI :(
> Would it be possible to make holding down the button scroll continuously

see bug #342906
Attachment #227541 - Flags: second-review?(mconnor) → second-review+
trunk:
mozilla/toolkit/content/xul.css 1.78
mozilla/toolkit/content/widgets/scrollbox.xml 1.9
mozilla/toolkit/content/widgets/tabbrowser.xml 1.161
mozilla/toolkit/themes/pinstripe/global/scrollbox.css  1.4
mozilla/toolkit/themes/winstripe/global/scrollbox.css 1.6
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 227541 [details] [diff] [review]
patch

We need this on 181b1 asap if bug 342906 is still a (181b1) blocker.
Attachment #227541 - Flags: approval1.8.1?
asaf, I've updated my trunk and rebuilt since your landing and I see a couple issues.

1)  context menus have scroll up / down buttons.
2)  the horizontal scrollbox buttons are bigger, and have more of a button like appearance and behavior (but this may be desirable, see bug #342909).  

I'll attach a couple screen shots.

Note, I've updated my entire tree, so these might not be regressions due to your change.
I don't see how this could be a regression of this change, as i've only removed click-to-scroll related stuff, going to check in my trunk build now.

The button look & feel was intended, I think. If this appears wrong we can tweak the style.
FWIW, i wasn't able to reproduce this in my mac trunk build (with cvs up in toolkit and browser only).
> The button look & feel was intended, I think. If this appears wrong we can
> tweak the style.

I think we should make them look and act like buttons (including disabling them, see bug #342841, if possible) when in "clicktoscroll" mode.

we should center the image in the button, too.  currently the arrow images are on the left within each button.
> 1)  context menus have scroll up / down buttons.

false alarm.  it may have been cvs-merge weirdness in my local tree.  after checking out toolkit and rebuilding again, things appear fine.
Comment on attachment 227964 [details]
screen shot of the context menu with arrows

ignore this screen shot.
Attachment #227964 - Attachment is obsolete: true
Target Milestone: mozilla1.8.1beta2 → mozilla1.8.1beta1
My bookmarks menu is too tall to fit on the screen.  Since this patch landed, I am unable to scroll to see all the bookmarks.
Bill Gianopoulos: Did you test this by backing out? A few other scrollbox fixes have landed today (which affect all kinds of scrollboxes), this one hardly touches the normal&vertical binding.
Attached patch regression fixSplinter Review
Fix autorepeat issue (see comment 21), thanks for pointing this out Bill.
Attachment #227989 - Flags: first-review?(sspitzer)
Comment on attachment 227989 [details] [diff] [review]
regression fix

r=sspitzer
Attachment #227989 - Flags: first-review?(sspitzer) → first-review+
Comment on attachment 227989 [details] [diff] [review]
regression fix

mozilla/toolkit/content/widgets/scrollbox.xml 1.10
Thanks.

Tested the fix.  Works great!
I've checked this in on trunk; will get a post-facto review.
Attachment #228039 - Flags: first-review?(mconnor)
Comment on attachment 228039 [details] [diff] [review]
Don't let the empty label take space

We don't need the same fix for pinstripe?
Attachment #228039 - Flags: first-review?(mconnor) → first-review+
Let's get this cleaned up
Flags: blocking-firefox2? → blocking-firefox2+
Attachment #228039 - Flags: approval1.8.1? → approval1.8.1+
Attachment #227989 - Flags: approval1.8.1? → approval1.8.1+
Attachment #227541 - Flags: approval1.8.1? → approval1.8.1+
Fixed on 1.8 branch for beta 1 (branch patch is on bug 318168).
Keywords: fixed1.8.1
Whiteboard: [have patch]
This bug's patch forgot to add the classes to the :hover and :hover:active blocks in scrollbar.css:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/themes/winstripe/global/scrollbox.css&rev=1.8&mark=94-103#80

I believe this is why there is NO hover feedback for the buttons in Windows without themes (e.g. Classic).
In fact, it's much worse than that. I'm trying to make a patch, though, but it's horrible down here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This makes sure that the autorepeatbutton visuals do not override the non-native visual appearance of toolbarbuttons that are used for scrollbuttons. In other works, it makes the tab scrolling buttons look right when there is no native theme in effect.
Attachment #228434 - Flags: second-review?(mconnor)
Attachment #228434 - Flags: first-review?(sspitzer)
Comment on attachment 228434 [details] [diff] [review]
Don't override non-native theme for scrollbuttons

r=mano.
Attachment #228434 - Flags: second-review?(mconnor)
Attachment #228434 - Flags: first-review?(sspitzer)
Attachment #228434 - Flags: first-review+
Attachment #228434 - Flags: approval1.8.1?
Silver, in future, please file separate bugs for regressions. Thanks for fixing this.
Keywords: fixed1.8.1
Target Milestone: mozilla1.8.1beta1 → mozilla1.8.1beta2
Asaf, this isn't technically a regression, as the buttons didn't used to have borders at all. Also, reopening gets way more attention and I'm not risking this getting lost under the carpet.

I notice you've removed the second-review; does that mean I can land it on trunk?
(In reply to comment #36)
> I notice you've removed the second-review; does that mean I can land it on
> trunk?

Please do. 

Comment on attachment 228434 [details] [diff] [review]
Don't override non-native theme for scrollbuttons

Landed on trunk.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Attachment #228434 - Flags: approval1.8.1? → approval1.8.1+
Comment on attachment 228434 [details] [diff] [review]
Don't override non-native theme for scrollbuttons

Landed on 1.8 branch.
Keywords: fixed1.8.1
Duplicate of this bug: 45186
You need to log in before you can comment on or make changes to this bug.