Status

()

Core
XUL
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: Neil Deakin (mostly unavailable until September), Assigned: Neil Deakin (mostly unavailable until September))

Tracking

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

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"/>
Created attachment 215614 [details] [diff] [review]
Support repeating button
Attachment #215614 - Flags: superreview?(roc)
Attachment #215614 - Flags: review?(neil)

Comment 2

12 years ago
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+

Comment 5

12 years ago
OK, but then shouldn't they repeat with the keyboard too?

Comment 6

12 years ago
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.

Comment 8

12 years ago
(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).
Created attachment 215782 [details] [diff] [review]
This patch doesn't change the button frame, and uses the syntax <button type="repeat"/>
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 10

12 years ago
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.
Created attachment 216127 [details] [diff] [review]
Implement what I described in the previous comment
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?

Comment 16

12 years ago
(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.

Updated

12 years ago
Attachment #216127 - Flags: review?(neil) → review+
So is the syntax for the repeat button here OK? Are more reviews necessary?

Updated

11 years ago
Blocks: 342906
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Attachment #216127 - Flags: superreview?(roc)
Attachment #215782 - Flags: superreview?(roc)
Depends on: 345318

Updated

9 years ago
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.