Last Comment Bug 331055 - Add a repeating button
: Add a repeating button
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Neil Deakin
:
Mentors:
http://wiki.mozilla.org/XUL:Specs:Num...
Depends on: 345318
Blocks: 342906
  Show dependency treegraph
 
Reported: 2006-03-19 16:25 PST by Neil Deakin
Modified: 2008-07-31 03:20 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Support repeating button (14.12 KB, patch)
2006-03-19 16:26 PST, Neil Deakin
roc: superreview+
Details | Diff | Review
This patch doesn't change the button frame, and uses the syntax <button type="repeat"/> (9.92 KB, patch)
2006-03-21 08:27 PST, Neil Deakin
neil: review+
Details | Diff | Review
Implement what I described in the previous comment (11.46 KB, patch)
2006-03-24 10:20 PST, Neil Deakin
neil: review+
Details | Diff | Review

Description Neil Deakin 2006-03-19 16:25:10 PST
Add a button where the command event fires repeatedly while the mouse is held down. This is needed for arrow buttons like in the spinbuttons.

<button type="repeat" label="OK"/>
Comment 1 Neil Deakin 2006-03-19 16:26:51 PST
Created attachment 215614 [details] [diff] [review]
Support repeating button
Comment 2 neil@parkwaycc.co.uk 2006-03-19 17:12:09 PST
Isn't that more like the scrollbarbutton behaviour? (I now notice you've provided a way to select which behaviour you want). Also I don't see why you need to add the key handling code, do you want spinbuttons to be focusable? If not, then calling them <spinbutton> would seem more reasonable and you could then use type="increment" or "decrement" on them.
Comment 3 Neil Deakin 2006-03-19 17:28:19 PST
I plan on removing the scrollbarbutton frame and just using the repeat button instead. But is much harder due to the mediators/listeners on the scrollbar.

While I'm planning on using the repeat button for spinbuttons, I don't want to prevent any other uses it might have for other widgets.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-03-19 19:45:10 PST
Comment on attachment 215614 [details] [diff] [review]
Support repeating button

Don't pass aPresContext to CheckKeys, just call GetPresContext() inside it instead.
Comment 5 neil@parkwaycc.co.uk 2006-03-20 00:22:32 PST
OK, but then shouldn't they repeat with the keyboard too?
Comment 6 neil@parkwaycc.co.uk 2006-03-20 04:20:47 PST
Comment on attachment 215614 [details] [diff] [review]
Support repeating button

> button {
>   -moz-binding: url("chrome://global/content/bindings/button.xml#button");
> }
> 
>+button[repeat] {
>+  -moz-binding: url("chrome://global/content/bindings/button.xml#button-repeat");
>+}
>+
> button[type="menu"] {
>   -moz-binding: url("chrome://global/content/bindings/button.xml#menu");
> }
> 
> button[type="menu-button"] {
>   -moz-binding: url("chrome://global/content/bindings/button.xml#menu-button");
> }
Since bindings are exclusive I think I'd prefer if you used type="repeat".

>+  // skip button frame handling to prevent click handling
>+  return nsBoxFrame::HandleEvent(aPresContext, aEvent, aEventStatus);
What's wrong with
case NS_MOUSE_LEFT_CLICK:
  // skip button frame handling to prevent click handling
  return nsBoxFrame::HandleEvent(aPresContext, aEvent, aEventStatus);
That would avoid changing nsButtonFrame.
Comment 7 Neil Deakin 2006-03-20 10:25:49 PST
> Since bindings are exclusive I think I'd prefer if you used type="repeat".

I also plan on adding another value in the future for handling mouse capture once all the view work is done, especially since scrollbar arrows should capture until the mouse is released.
Comment 8 neil@parkwaycc.co.uk 2006-03-20 15:43:01 PST
(In reply to comment #7)
>I also plan on adding another value in the future for handling mouse capture
Is this value specific to repeating buttons or would it apply to all buttons...

>scrollbar arrows should capture until the mouse is released.
...assuming you can come up with an argument for optional capturing
(excluding autorepeatbuttons as you don't even have to click them).
Comment 9 Neil Deakin 2006-03-21 08:27:42 PST
Created attachment 215782 [details] [diff] [review]
This patch doesn't change the button frame, and uses the syntax <button type="repeat"/>
Comment 10 neil@parkwaycc.co.uk 2006-03-22 09:47:45 PST
Comment on attachment 215782 [details] [diff] [review]
This patch doesn't change the button frame, and uses the syntax <button type="repeat"/>

When trying this out it conflicted with a change I'd made for some reason (my guess is that it was intended for dragging into a scrolling popup e.g. rearranging a long bookmarks menu) which was to make autorepeat buttons repeat for drag enter/exit events as well as normal mouse events. Does that sound reasonable to you or shall I just leave it in as a local change until I remember what it was for?
Comment 11 Neil Deakin 2006-03-22 10:13:02 PST
Sounds like it might be better to include it in a patch about dragging in long menus. I can test the change out if you like though.

Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-03-22 12:25:13 PST
type="repeat" doesn't seem like good API to me. Can we just do something to address the comment in InitRepeatMode() now? How about repeat="active" and repeat="hover"?
Comment 13 Neil Deakin 2006-03-23 08:08:50 PST
Perhaps <button type="repeat" repeat="active|hover"> is OK, where active is the default? I could add repeat="hover" into the autorepeatbutton binding so it defaults to hover.
Comment 14 Neil Deakin 2006-03-24 10:20:44 PST
Created attachment 216127 [details] [diff] [review]
Implement what I described in the previous comment
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-03-24 13:37:52 PST
+  nsAutoString repeat;
+  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::repeat, repeat);
+  mIsPressMode = !repeat.EqualsLiteral("hover");

AttrValueIs?

Why do you want to have type="repeat", isn't it a bit redundant if we have a repeat attribute?
Comment 16 neil@parkwaycc.co.uk 2006-03-24 13:50:34 PST
(In reply to comment #15)
>Why do you want to have type="repeat", isn't it a bit redundant if we have a
>repeat attribute?
That was my fault, we already have <button type="menu"> etc. The only widgets that come to mind that don't use type are <menulist editable> and <textbox multiline="true"> and even then all the other types of textbox (which do use the type attribute) are all single-line anyway.
Comment 17 Neil Deakin 2006-05-16 13:41:29 PDT
So is the syntax for the repeat button here OK? Are more reviews necessary?

Note You need to log in before you can comment on or make changes to this bug.