Closed Bug 1453421 Opened 5 years ago Closed 5 years ago

Find bar shouldn't loop over all event properties, triggering sync reflows from e.g. event.rangeOffset

Categories

(Toolkit :: Find Toolbar, enhancement)

Product:
Toolkit
Component:
Find Toolbar
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Whiteboard: [fxperf:p2])

Attachments

(1 file)

Bug 1453421 - stop findbar from looping over every single property,
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details 
As in summary. Instead, we should have a list of properties we actually need in the parent to make decisions. These should be pretty limited (effectively modifier keys, charCode and/or 'key', I expect). Then we just send those properties. That will be faster + won't trigger layout/style flushes.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8968853 [details]
Bug 1453421 - stop findbar from looping over every single property,

https://reviewboard.mozilla.org/r/237572/#review243300

I put two things down for your consideration. Thanks!

::: toolkit/content/browser-content.js:1077
(Diff revision 1)
>    _passKeyToParent(event) {
>      event.preventDefault();
> +    // These are the properties required to dispatch another 'real' event
> +    // to the findbar in the parent in _dispatchKeypressEvent in findbar.xml .
> +    // If you make changes here, verify that that method can still do its job.
> +    let requiredProps = [

nit: dunno if it matters already, but s/let/const/

::: toolkit/content/widgets/findbar.xml:776
(Diff revision 1)
> -        <parameter name="aEvent"/>
> +        <parameter name="fakeEvent"/>
>          <body><![CDATA[
>            if (!aTarget)
>              return;
>  
>            let event = document.createEvent("KeyboardEvent");

nit: while you're here, maybe you'd like to modernize this to use KeyboardEvent instead?
```js
const global = aTarget.ownerGlobal;
aTarget.dispatchEvent(new global.KeyboardEvent(fakeEvent.type, fakeEvent));
```
Attachment #8968853 - Flags: review?(mdeboer) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/697d0f7076eb
stop findbar from looping over every single property, r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/697d0f7076eb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.