Closed Bug 1429573 Opened 6 years ago Closed 6 years ago

Simplify textbox[type="number"] implementation

Categories

(Toolkit :: XUL Widgets, task)

Product:
Toolkit
Component:
XUL Widgets
Type:
task
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

Bug 1429573 - Use input[type=number] in textbox[type=number] implementation.
59 bytes, text/x-review-board-request
surkov
: review+
Paolo
: review+
Details
Bug 1429573 - Remove spinbuttons.xml bindings now that they are unused.
59 bytes, text/x-review-board-request
Paolo
: review+
Details
Before/After in preferences
15.58 KB, image/png
Details 
We can simplify the implementation by using input[type=number].

The decimalPlaces handling is unused, so that will be removed here as well.


Once that is done, the spinbuttons binding can be removed.
Comment on attachment 8941578 [details]
Bug 1429573 - Use input[type=number] in textbox[type=number] implementation.

https://reviewboard.mozilla.org/r/211840/#review217660

::: toolkit/content/tests/chrome/test_textbox_number.xul:97
(Diff revision 2)
>    // check read only state
>    n1.readOnly = true;
>    n1.min = -10;
>    n1.max = 15;
>    n1.value = 12;
>    // no events should fire and no changes should occur when the field is read only
>    synthesizeKeyExpectEvent("VK_UP", { }, n1, "!change", "key up read only");
>    is(n1.value, "12", "key up read only value");
>    synthesizeKeyExpectEvent("VK_DOWN", { }, n1, "!change", "key down read only");
>    is(n1.value, "12", "key down read only value");

Note to self: This is probably testing absolutely nothing, we should either remove this block (as it is covered by input[type=number] tests), either force a focus on the number input before emulating the keyboard events.

::: toolkit/themes/shared/in-content/common.inc.css:350
(Diff revision 2)
> -xul|*.spinbuttons-button {
> +html|*.numberbox-input::-moz-number-spin-up,
> +html|*.numberbox-input::-moz-number-spin-down {
> +  -moz-appearance: none;
>    min-height: initial;
>    margin-inline-start: 10px !important;
>    margin-inline-end: 2px !important;
> +  padding: 1px 5px 2px !important;
>  }
>  
> -xul|*.spinbuttons-up {
> +html|*.numberbox-input::-moz-number-spin-up {
>    margin-top: 2px !important;
>    border-radius: 1px 1px 0 0;
> +  background-image: url("chrome://global/skin/arrow/arrow-up.gif");
>  }
>  
> -xul|*.spinbuttons-down  {
> +html|*.numberbox-input::-moz-number-spin-down  {
>    margin-bottom: 2px !important;
>    border-radius: 0 0 1px 1px;
> +  background-image: url("chrome://global/skin/arrow/arrow-dn.gif");
>  }
>  
> -xul|*.spinbuttons-button > xul|*.button-box {
> -  padding: 1px 5px 2px !important;
> +html|*.numberbox-input:disabled::-moz-number-spin-up,
> +html|*.numberbox-input:disabled::-moz-number-spin-down {

Note to self: this will need platform changes to actually work, since the pseudo elements are only exposed to UA sheets.
Depends on: 1433389
Depends on: 1417708, 1429625
Attachment #8941578 - Flags: review?(paolo.mozmail)
Try is pretty much green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=34e5d5a69359&selectedJob=158650820

The remaining failures (browser_parsable_css.js & browser_all_files_referenced.js) depend on bug 1433389 getting fixed.
Comment on attachment 8941578 [details]
Bug 1429573 - Use input[type=number] in textbox[type=number] implementation.

https://reviewboard.mozilla.org/r/211840/#review221658

You may want to ask for accessibility review for the changes to test_txtctrl.xul, unless you're confident that they don't impact accessibility.

::: toolkit/content/tests/chrome/test_textbox_number.xul:9
(Diff revision 4)
> -  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>  
> +  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
>    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>  

nit: either change whitespace on both lines or neither

::: toolkit/content/widgets/numberbox.xml:145
(Diff revision 4)
> -      </handler>
> -
>        <handler event="change">
> -        if (event.originalTarget == this.inputField) {
> -          var newval = this.inputField.value;
> -          this._validateValue(newval);
> +        // Support focus events for input[type=number] where the originalTarget is an anonymous
> +        // input inside the real input.
> +        let originalTarget = event.originalTarget.closest("[anonid=input]") || event.originalTarget;

Is this needed for "change" events at all? Is there any case where we won't find the [anonid=input] element?

::: toolkit/content/widgets/textbox.xml:130
(Diff revision 4)
> +          // https://html.spec.whatwg.org/#do-not-apply
> +          if (this.inputField.type === "text"
> +           || this.inputField.tagName === "html:textarea") {

nit: I think || goes at the end

::: toolkit/content/widgets/textbox.xml:197
(Diff revision 4)
>            if (this.hasAttribute("focused"))
>              return;
>  
> -          switch (event.originalTarget) {
> +          // Support focus events for input[type=number] where the originalTarget is an anonymous
> +          // input inside the real input.
> +          let originalTarget = event.originalTarget.closest("[anonid=input]") || event.originalTarget;

I'm not too confident about reviewing what is going on here, but "closest" is not the best choice because it may try to look up all the ancestors.

What is this code trying to do, and which cases does it need to handle?

::: toolkit/themes/shared/in-content/common.inc.css
(Diff revision 4)
> -xul|*.spinbuttons-up > xul|*.button-box > xul|*.button-icon {
> -  list-style-image: url("chrome://global/skin/arrow/arrow-up.gif");
> -}
> -
> -xul|*.spinbuttons-up[disabled="true"] > xul|*.button-box > xul|*.button-icon {
> -  list-style-image: url("chrome://global/skin/arrow/arrow-up-dis.gif");

Looks like arrow-up-dis.gif and arrow-dn-dis.gif will become unused on Linux?
Attachment #8941578 - Flags: review?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #7)
> Comment on attachment 8941578 [details]
> Bug 1429573 - Simplify textbox[type=number] implementation.
> 
> https://reviewboard.mozilla.org/r/211840/#review221658
> 
> You may want to ask for accessibility review for the changes to
> test_txtctrl.xul, unless you're confident that they don't impact
> accessibility.
> 
> ::: toolkit/content/tests/chrome/test_textbox_number.xul:9
> (Diff revision 4)
> > -  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>  
> > +  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
> >    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>  
> 
> nit: either change whitespace on both lines or neither
> 
> ::: toolkit/content/widgets/numberbox.xml:145
> (Diff revision 4)
> > -      </handler>
> > -
> >        <handler event="change">
> > -        if (event.originalTarget == this.inputField) {
> > -          var newval = this.inputField.value;
> > -          this._validateValue(newval);
> > +        // Support focus events for input[type=number] where the originalTarget is an anonymous
> > +        // input inside the real input.
> > +        let originalTarget = event.originalTarget.closest("[anonid=input]") || event.originalTarget;
> 
> Is this needed for "change" events at all? Is there any case where we won't
> find the [anonid=input] element?
> 
> ::: toolkit/content/widgets/textbox.xml:130
> (Diff revision 4)
> > +          // https://html.spec.whatwg.org/#do-not-apply
> > +          if (this.inputField.type === "text"
> > +           || this.inputField.tagName === "html:textarea") {
> 
> nit: I think || goes at the end
> 
> ::: toolkit/content/widgets/textbox.xml:197
> (Diff revision 4)
> >            if (this.hasAttribute("focused"))
> >              return;
> >  
> > -          switch (event.originalTarget) {
> > +          // Support focus events for input[type=number] where the originalTarget is an anonymous
> > +          // input inside the real input.
> > +          let originalTarget = event.originalTarget.closest("[anonid=input]") || event.originalTarget;
> 
> I'm not too confident about reviewing what is going on here, but "closest"
> is not the best choice because it may try to look up all the ancestors.

input[type=number] is structured like this:
<input type=number>
  <div>
    <input type=text>
    <div>
      <div role="button"/>
      <div role="button"/>
    </div>
  </div>
</input>

event.originalTarget gets the inner text input on focus, which causes the `focused` attribute not to be added on textbox[type=number]. This is what the code is fixing.

> Is this needed for "change" events at all? Is there any case where we won't find the [anonid=input] element?

I think it's probably not needed for "change" events, but I'm not actually sure here.

> You may want to ask for accessibility review for the changes to test_txtctrl.xul

Will do, I mainly copied the accessibility tree from the <input type=number> a11y test.
Hm, maybe we'll want to extract this bit to a method that we override in the numberbox binding, if there is a chance that "closest" doesn't find the element. Other solutions welcome too.
New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65e36a9b2574cf6931aae1c5b59b90dbdb7ba681

Which should hopefully be 100% green with bug 1433389 fixed.


(In reply to :Paolo Amadini from comment #9)
> Hm, maybe we'll want to extract this bit to a method that we override in the
> numberbox binding, if there is a chance that "closest" doesn't find the
> element. Other solutions welcome too.

It doesn't seem too clean to duplicate the focus logic :/

I left this piece of code for now, but I'll change it if we find a better solution.
Comment on attachment 8941578 [details]
Bug 1429573 - Use input[type=number] in textbox[type=number] implementation.

https://reviewboard.mozilla.org/r/211840/#review224220

I assume you tested manually the new implementation of the increment and decrement functions.

::: toolkit/content/tests/chrome/test_textbox_number.xul
(Diff revision 10)
> -  var nsbrect = $("n8").spinButtons.getBoundingClientRect();
> -  ok(nsbrect.left == 0 && nsbrect.top == 0 && nsbrect.right == 0, nsbrect.bottom == 0,
> -     "hidespinbuttons");

Can we still have an automated test for the "hidespinbuttons" case?

::: toolkit/content/widgets/textbox.xml:197
(Diff revision 10)
>            if (this.hasAttribute("focused"))
>              return;
>  
> -          switch (event.originalTarget) {
> +          // Support focus events for input[type=number] where the originalTarget is an anonymous
> +          // input inside the real input.
> +          let originalTarget = event.originalTarget.closest("[anonid=input]") || event.originalTarget;

What you suggest in comment 19 is fine, we don't necessarily need a dedicated method and we can give the base control has knowledge about the structure of the derived control.

However, this implementation still has to be improved. For example, in case originalTarget is "this", the "closest" call would always look up all the ancestors and then fall back to originalTarget.

::: toolkit/content/widgets/textbox.xml:239
(Diff revision 10)
>          <![CDATA[
>            this.mIgnoreClick = this.hasAttribute("focused");
>  
>            if (!this.mIgnoreClick) {
>              this.mIgnoreFocus = true;
> -            this.inputField.setSelectionRange(0, 0);
> +            this.setSelectionRange(0, 0);

Not that it matters too much, but this is a change with the behavior of selection on click, correct?

Worth mentioning any behavioral changes in the commit message.

::: toolkit/themes/osx/global/in-content/common.css
(Diff revision 10)
> -xul|*.spinbuttons-button > xul|*.button-box {
> -  padding-inline-start: 2px !important;
> -  padding-inline-end: 3px !important;
> -}
> -
> -xul|*.spinbuttons-button > xul|*.button-box > xul|*.button-text {
> -  display: none;
> -}
> -

Do these in-content buttons look exactly the same? Can you post a screenshot of the styling differences, if any?
Attachment #8941578 - Flags: review?(paolo.mozmail)
Comment on attachment 8948960 [details]
Bug 1429573 - Remove spinbuttons.xml bindings now that they are unused.

https://reviewboard.mozilla.org/r/218384/#review224226
Attachment #8948960 - Flags: review?(paolo.mozmail) → review+
Top is before (it looks broken atm), bottom is after.
> Can we still have an automated test for the "hidespinbuttons" case?

This is still covered by XUL reftests.
The newest revision will only call closest() if the originalTarget anonid isn't input.
Comment on attachment 8941578 [details]
Bug 1429573 - Use input[type=number] in textbox[type=number] implementation.

https://reviewboard.mozilla.org/r/211840/#review224366

r=me for a11y test change, the hierarchy should be ok, at least it stays close to ARIA example http://www.oaa-accessibility.org/example/33/ but I would double check with Marco
Attachment #8941578 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8941578 [details]
Bug 1429573 - Use input[type=number] in textbox[type=number] implementation.

Marco, does a spinbutton role containing text entry and up/down buttons work out?
Attachment #8941578 - Flags: feedback?(mzehe)
Note that for the a11y tree, I copied same tree used by input[type=number]: https://searchfox.org/mozilla-central/source/accessible/tests/mochitest/tree/test_formctrl.html#65-72
Comment on attachment 8941578 [details]
Bug 1429573 - Use input[type=number] in textbox[type=number] implementation.

lgtm.
Attachment #8941578 - Flags: feedback?(mzehe) → feedback+
(In reply to Tim Nguyen :ntim from comment #35)
> Note that for the a11y tree, I copied same tree used by input[type=number]:
> https://searchfox.org/mozilla-central/source/accessible/tests/mochitest/tree/
> test_formctrl.html#65-72

right, that makes sense. However XUL version has section as a root, which I believe is not big deal in terms of AT support, but not a good thing in terms of consistency.
Comment on attachment 8941578 [details]
Bug 1429573 - Use input[type=number] in textbox[type=number] implementation.

https://reviewboard.mozilla.org/r/211840/#review224510

::: toolkit/content/widgets/textbox.xml:130
(Diff revision 13)
>  
>        <method name="setSelectionRange">
>          <parameter name="aSelectionStart"/>
>          <parameter name="aSelectionEnd"/>
>          <body>
> +          // https://html.spec.whatwg.org/#do-not-apply

Please put a sentence here.

::: toolkit/content/widgets/textbox.xml:132
(Diff revision 13)
>          <parameter name="aSelectionStart"/>
>          <parameter name="aSelectionEnd"/>
>          <body>
> +          // https://html.spec.whatwg.org/#do-not-apply
> +          if (this.inputField.type === "text" ||
> +              this.inputField.tagName === "html:textarea") {

nit: use == like we do in most places. The type strictness doesn't buy you anything here.

::: toolkit/content/widgets/textbox.xml:198
(Diff revision 13)
>              return;
>  
> -          switch (event.originalTarget) {
> +          // Support focus events for input[type=number] where the originalTarget is an anonymous
> +          // input inside the real input.
> +          let { originalTarget } = event;
> +          if (originalTarget.getAttribute("anonid") !== "input") {

ditto

::: toolkit/content/xul.css:799
(Diff revision 13)
>    -moz-binding: url("chrome://global/content/bindings/numberbox.xml#numberbox");
>  }
>  
> +textbox[type="number"][hidespinbuttons="true"] > .numberbox-input {
> +  -moz-appearance: textfield !important;
> +}

This looks like it belongs in numberbox.css.
Comment on attachment 8941578 [details]
Bug 1429573 - Use input[type=number] in textbox[type=number] implementation.

https://reviewboard.mozilla.org/r/211840/#review224520

::: toolkit/content/widgets/textbox.xml:202
(Diff revision 13)
> +          switch (originalTarget) {
>              case this:
>                // Forward focus to actual HTML input
>                this.inputField.focus();
>                break;
>              case this.inputField:
>                if (this.mIgnoreFocus) {
>                  this.mIgnoreFocus = false;
>                } else if (this.clickSelectsAll) {
>                  try {
>                    if (!this.editor || !this.editor.composing)
>                      this.editor.selectAll();
>                  } catch (e) {}
>                }
>                break;
>              default:
>                // Allow other children (e.g. URL bar buttons) to get focus
>                return;
>            }
>            this.setAttribute("focused", "true");

So, event.originalTarget can point to any element from this structure:

textbox: this
  element: custom child, can be anything including another textbox
  input[type="number"][anonid="input"]: this.inputField
    div
      input: the inner input
    ...spinbuttons...

The logic should be something like:

- If originalTarget is "this":
   - Set "focused" and do what "case this:" does
- If originalTarget is "this.inputField" or the inner input
   - Set "focused" and do what "case this.inputField:" does
- Otherwise, do nothing.

The code as implemented now sort of works, but focusing any custom child will still look at all the ancestors of the binding, because "closest" will not stop until the root element.

You can probably rewrite the switch statement to use if statements, and check "this.inputField.contains(originalTarget)" as the second condition. You can check originalTarget.localName == "input" if you need to exclude the spinbuttons.
Attachment #8941578 - Flags: review?(paolo.mozmail)
Thanks for the review!

Unfortunately, this.inputField.contains(originalTarget) doesn't work, because the child node here (originalTarget) is anonymous.

I also tried `document.getAnonymousElementByAttribute(this.inputField, "type", "text") == originalTarget` but it also doesn't work.

Any suggestions ?
Flags: needinfo?(paolo.mozmail)
(In reply to Tim Nguyen :ntim from comment #40)
> Any suggestions ?

When originalTarget is not "this", maybe you can check if originalTarget.parentNode.parentNode is "this.inputField"? It's like using "closest" but stopping two levels up. This depends on the structure of the HTML number input, but should be fine since if it's covered by a regression tests, which should be the case anyways.
Flags: needinfo?(paolo.mozmail)
Comment on attachment 8941578 [details]
Bug 1429573 - Use input[type=number] in textbox[type=number] implementation.

https://reviewboard.mozilla.org/r/211840/#review224756

::: toolkit/content/widgets/textbox.xml:204
(Diff revision 15)
> -              this.inputField.focus();
> +            this.inputField.focus();
> -              break;
> -            case this.inputField:
> +            this.setAttribute("focused", "true");
> +            return;
> +          }
> +
> +          // Support input[type=number] where originalTarget corresponds to the anonymous child input.

"where originalTarget can be" is probably more accurate.
Comment on attachment 8941578 [details]
Bug 1429573 - Use input[type=number] in textbox[type=number] implementation.

https://reviewboard.mozilla.org/r/211840/#review224754

Thanks!

::: toolkit/content/widgets/textbox.xml:204
(Diff revision 14)
> +          if (originalTarget == this.inputField ||
> +              originalTarget.parentNode.parentNode == this.inputField) {

Add a comment for why we're checking the parent nodes here. Also mentioning that originalTarget.parentNode can't be null because there's at least one parent element.

Also, I guess you don't need to test for originalTarget.localName == "input" to exclude the spinbuttons, right?
Attachment #8941578 - Flags: review?(paolo.mozmail) → review+
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1c398da94994
Use input[type=number] in textbox[type=number] implementation. r=Paolo,surkov
https://hg.mozilla.org/integration/autoland/rev/bd6892535d35
Remove spinbuttons.xml bindings now that they are unused. r=Paolo
Backed out for reftest failures 
Push : https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=bd6892535d35996fd3186885d51dc78d5837f358 

Analyzer: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/Q-YP3lySRi65JfufFS1v-Q/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

[task 2018-02-09T14:56:48.945Z] 14:56:48     INFO - REFTEST TEST-START | file:///builds/worker/workspace/build/tests/reftest/tests/editor/reftests/xul/number-3.xul == file:///builds/worker/workspace/build/tests/reftest/tests/editor/reftests/xul/number-ref.xul
[task 2018-02-09T14:56:48.947Z] 14:56:48     INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/reftest/tests/editor/reftests/xul/number-3.xul | 1674 / 1967 (85%)
[task 2018-02-09T14:56:49.062Z] 14:56:49     INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///builds/worker/workspace/build/tests/reftest/tests/editor/reftests/xul/number-3.xul == file:///builds/worker/workspace/build/tests/reftest/tests/editor/reftests/xul/number-ref.xul | image comparison, max difference: 192, number of differing pixels: 24593
[task 2018-02-09T14:56:49.071Z] 14:56:49     INFO - REFTEST INFO | Saved log: START file:///builds/worker/workspace/build/tests/reftest/tests/editor/reftests/xul/number-3.xul
[task 2018-02-09T14:56:49.072Z] 14:56:49     INFO - REFTEST INFO | Saved log: [CONTENT] OnDocumentLoad triggering AfterOnLoadScripts
[task 2018-02-09T14:56:49.072Z] 14:56:49     INFO - REFTEST INFO | Saved log: Initializing canvas snapshot
[task 2018-02-09T14:56:49.072Z] 14:56:49     INFO - REFTEST INFO | Saved log: DoDrawWindow 0,0,800,1000
[task 2018-02-09T14:56:49.072Z] 14:56:49     INFO - REFTEST INFO | Saved log: [CONTENT] RecordResult fired
[task 2018-02-09T14:56:49.072Z] 14:56:49     INFO - REFTEST INFO | Saved log: RecordResult fired
[task 2018-02-09T14:56:49.072Z] 14:56:49     INFO - REFTEST INFO | Saved log: RecordResult fired
[task 2018-02-09T14:56:49.073Z] 14:56:49     INFO - REFTEST TEST-END | file:///builds/worker/workspace/build/tests/reftest/tests/editor/reftests/xul/number-3.xul == file:///builds/worker/workspace/build/tests/reftest/tests/editor/reftests/xul/number-ref.xul
Flags: needinfo?(ntim.bugs)
Backout by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/51625dfd3a71
Backed out 2 changesets for reftest failures /tests/reftest/tests/editor/reftests/xul/number-3.xul. on a CLOSED TREE
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fe69b415f45b
Use input[type=number] in textbox[type=number] implementation. r=Paolo,surkov
https://hg.mozilla.org/integration/autoland/rev/75364898f5f6
Remove spinbuttons.xml bindings now that they are unused. r=Paolo
Backed out 2 changesets (bug 1429573) for failing reftest on reftest/tests/editor/reftests/xul/number-3.xul on a CLOSED TREE
Link of the failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=161400161&repo=autoland&lineNumber=23912
Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=14128b25db9fab9e597935876a318ac927f21ef2
Backout revision 14128b25db9f
Failing push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=75364898f5f63633d8c42a6aff66a185c36cc084

:ntim this is the backout info, could you please take a look?
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/81891dee83da
Use input[type=number] in textbox[type=number] implementation. r=Paolo,surkov
https://hg.mozilla.org/integration/autoland/rev/fe8269ac41f5
Remove spinbuttons.xml bindings now that they are unused. r=Paolo
Flags: needinfo?(ntim.bugs)
https://hg.mozilla.org/mozilla-central/rev/81891dee83da
https://hg.mozilla.org/mozilla-central/rev/fe8269ac41f5
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1437284
Depends on: 1437286
Depends on: 1437302
Depends on: 1451804
Depends on: 1459563
Blocks: 1437641
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.