Closed
Bug 331055
Opened 19 years ago
Closed 18 years ago
Add a repeating button
Categories
(Core :: XUL, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: enndeakin, Assigned: enndeakin)
References
()
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
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"/>
Assignee | ||
Comment 1•19 years ago
|
||
Attachment #215614 -
Flags: superreview?(roc)
Attachment #215614 -
Flags: review?(neil)
Comment 2•19 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.
Assignee | ||
Comment 3•19 years ago
|
||
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•19 years ago
|
||
OK, but then shouldn't they repeat with the keyboard too?
Comment 6•19 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.
Assignee | ||
Comment 7•19 years ago
|
||
> 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•19 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).
Assignee | ||
Comment 9•19 years ago
|
||
Attachment #215614 -
Attachment is obsolete: true
Attachment #215782 -
Flags: superreview?(roc)
Attachment #215782 -
Flags: review?
Attachment #215614 -
Flags: review?(neil)
Assignee | ||
Updated•19 years ago
|
Attachment #215782 -
Flags: review? → review?(neil)
Comment 10•19 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+
Assignee | ||
Comment 11•19 years ago
|
||
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"?
Assignee | ||
Comment 13•19 years ago
|
||
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.
Assignee | ||
Comment 14•19 years ago
|
||
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•19 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•19 years ago
|
Attachment #216127 -
Flags: review?(neil) → review+
Assignee | ||
Comment 17•19 years ago
|
||
So is the syntax for the repeat button here OK? Are more reviews necessary?
Assignee | ||
Updated•18 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•18 years ago
|
Attachment #216127 -
Flags: superreview?(roc)
Assignee | ||
Updated•18 years ago
|
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.
Description
•