Closed Bug 1476611 Opened 6 years ago Closed 6 years ago

Flatten the "richlistbox" bindings

Categories

(Toolkit :: UI Widgets, task, P3)

Product:
Toolkit
Component:
UI Widgets
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(2 files)

Bug 1476611 - Part 2 - Flatten the "richlistbox" bindings.
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1476611 - Part 1 - Remove unused listbox methods.
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
Once bug 1472555 lands, it will be possible to flatten the "richlistbox" bindings and remove "listbox.xml" entirely.
Comment on attachment 8994173 [details]
Bug 1476611 - Part 2 - Flatten the "richlistbox" bindings.

https://reviewboard.mozilla.org/r/258778/#review265664


Code analysis found 7 defects in this patch:
 - 7 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/content/widgets/richlistbox.xml:263
(Diff revision 1)
> +        ]]>
> +        </setter>
> +      </property>
> +
> +      <!-- nsIDOMXULMultiSelectControlElement -->
> +      <field name="selectedItems">new ChromeNodeList()</field>

Error: 'ChromeNodeList' is not defined. [eslint: no-undef]

::: toolkit/content/widgets/richlistbox.xml:853
(Diff revision 1)
> +      <handler event="keypress" phase="target">
> +        <![CDATA[
> +          if (this.disableKeyNavigation || !event.charCode ||
> +              event.altKey || event.ctrlKey || event.metaKey)
> +            return;
> +  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/content/widgets/richlistbox.xml:856
(Diff revision 1)
> +              event.altKey || event.ctrlKey || event.metaKey)
> +            return;
> +  
> +          if (event.timeStamp - this._lastKeyTime > 1000)
> +            this._incrementalString = "";
> +  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/content/widgets/richlistbox.xml:860
(Diff revision 1)
> +            this._incrementalString = "";
> +  
> +          var key = String.fromCharCode(event.charCode).toLowerCase();
> +          this._incrementalString += key;
> +          this._lastKeyTime = event.timeStamp;
> +  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/content/widgets/richlistbox.xml:866
(Diff revision 1)
> +          // If all letters in the incremental string are the same, just
> +          // try to match the first one
> +          var incrementalString = /^(.)\1+$/.test(this._incrementalString) ?
> +                                  RegExp.$1 : this._incrementalString;
> +          var length = incrementalString.length;
> +  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/content/widgets/richlistbox.xml:874
(Diff revision 1)
> +          var start = l > 0 ? this.getIndexOfItem(this.selectedItems[l - 1]) : -1;
> +          // start from the first element if none was selected or from the one
> +          // following the selected one if it's a new or a repeated-letter search
> +          if (start == -1 || length == 1)
> +            start++;
> +  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/content/widgets/richlistbox.xml:1073
(Diff revision 1)
> +          } else {
> +            /* We want to deselect all the selected items except what was
> +              clicked, UNLESS it was a right-click.  We have to do this
> +              in click rather than mousedown so that you can drag a
> +              selected group of items */
> +  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]
Comment on attachment 8994173 [details]
Bug 1476611 - Part 2 - Flatten the "richlistbox" bindings.

https://reviewboard.mozilla.org/r/258778/#review265666


Code analysis found 7 defects in this patch:
 - 7 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/content/widgets/richlistbox.xml:263
(Diff revision 2)
> +        ]]>
> +        </setter>
> +      </property>
> +
> +      <!-- nsIDOMXULMultiSelectControlElement -->
> +      <field name="selectedItems">new ChromeNodeList()</field>

Error: 'ChromeNodeList' is not defined. [eslint: no-undef]

::: toolkit/content/widgets/richlistbox.xml:853
(Diff revision 2)
> +      <handler event="keypress" phase="target">
> +        <![CDATA[
> +          if (this.disableKeyNavigation || !event.charCode ||
> +              event.altKey || event.ctrlKey || event.metaKey)
> +            return;
> +  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/content/widgets/richlistbox.xml:856
(Diff revision 2)
> +              event.altKey || event.ctrlKey || event.metaKey)
> +            return;
> +  
> +          if (event.timeStamp - this._lastKeyTime > 1000)
> +            this._incrementalString = "";
> +  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/content/widgets/richlistbox.xml:860
(Diff revision 2)
> +            this._incrementalString = "";
> +  
> +          var key = String.fromCharCode(event.charCode).toLowerCase();
> +          this._incrementalString += key;
> +          this._lastKeyTime = event.timeStamp;
> +  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/content/widgets/richlistbox.xml:866
(Diff revision 2)
> +          // If all letters in the incremental string are the same, just
> +          // try to match the first one
> +          var incrementalString = /^(.)\1+$/.test(this._incrementalString) ?
> +                                  RegExp.$1 : this._incrementalString;
> +          var length = incrementalString.length;
> +  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/content/widgets/richlistbox.xml:874
(Diff revision 2)
> +          var start = l > 0 ? this.getIndexOfItem(this.selectedItems[l - 1]) : -1;
> +          // start from the first element if none was selected or from the one
> +          // following the selected one if it's a new or a repeated-letter search
> +          if (start == -1 || length == 1)
> +            start++;
> +  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: toolkit/content/widgets/richlistbox.xml:1073
(Diff revision 2)
> +          } else {
> +            /* We want to deselect all the selected items except what was
> +              clicked, UNLESS it was a right-click.  We have to do this
> +              in click rather than mousedown so that you can drag a
> +              selected group of items */
> +  

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]
Comment on attachment 8994173 [details]
Bug 1476611 - Part 2 - Flatten the "richlistbox" bindings.

https://reviewboard.mozilla.org/r/258778/#review265690

I wish we could see cross-file line moves in review tools - I've spot checked but am assuming this is a direct copy/paste fold. Let me know if there's anything more going on with this that you want me to review.
Attachment #8994173 - Flags: review?(bgrinstead) → review+
Comment on attachment 8994174 [details]
Bug 1476611 - Part 1 - Remove unused listbox methods.

https://reviewboard.mozilla.org/r/258780/#review265692
Attachment #8994174 - Flags: review?(bgrinstead) → review+
CCing some Thunderbird folks - AFAICT the richlistbox methods being removed in part 1 are either unused in c-c or are replicated in separate bindings files. Part 2 removes listbox-base and listitem-base since there's now only one binding extending them in m-c, so if you have other bindings that extend them you may want to restore the bindings in c-c.
Depends on: 1477777
Filed bug 1477777 for the C-C work. Thanks for the heads-up.
(In reply to Brian Grinstead [:bgrins] from comment #7)
> I wish we could see cross-file line moves in review tools - I've spot
> checked but am assuming this is a direct copy/paste fold.

Yes, the second part just moves methods across files. In the first part I accidentally removed two methods that were still used, I've re-added them and started a tryserver build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d7cdd7890a5218588a68c74d368190372ab8bfd
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/162e063abd54
Part 1 - Remove unused listbox methods. r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0f8e3ef3a72
Part 2 - Flatten the "richlistbox" bindings. r=bgrins
https://hg.mozilla.org/mozilla-central/rev/162e063abd54
https://hg.mozilla.org/mozilla-central/rev/a0f8e3ef3a72
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1479215
Assignee: nobody → paolo.mozmail
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: