Closed Bug 331055 Opened 18 years ago Closed 18 years ago

Add a repeating button

Categories

(Core :: XUL, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: enndeakin, Assigned: enndeakin)

References

()

Details

Attachments

(2 files, 1 obsolete file)

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"/>
Attached patch Support repeating button (obsolete) — Splinter Review
Attachment #215614 - Flags: superreview?(roc)
Attachment #215614 - Flags: review?(neil)
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.
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.
Status: NEW → ASSIGNED
Comment on attachment 215614 [details] [diff] [review]
Support repeating button

Don't pass aPresContext to CheckKeys, just call GetPresContext() inside it instead.
Attachment #215614 - Flags: superreview?(roc) → superreview+
OK, but then shouldn't they repeat with the keyboard too?
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.
> 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.
(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).
Attachment #215614 - Attachment is obsolete: true
Attachment #215782 - Flags: superreview?(roc)
Attachment #215782 - Flags: review?
Attachment #215614 - Flags: review?(neil)
Attachment #215782 - Flags: review? → review?(neil)
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?
Attachment #215782 - Flags: review?(neil) → review+
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.

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"?
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.
Attachment #216127 - Flags: superreview?(roc)
Attachment #216127 - Flags: review?(neil)
+  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?
(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.
Attachment #216127 - Flags: review?(neil) → review+
So is the syntax for the repeat button here OK? Are more reviews necessary?
Blocks: 342906
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Attachment #216127 - Flags: superreview?(roc)
Attachment #215782 - Flags: superreview?(roc)
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.