onresize events should fire at every resize while resizing the window, not just at 200ms intervals

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: bugzilla, Assigned: smaug)

Tracking

(Depends on 1 bug)

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(status1.9.1 wontfix)

Details

(Whiteboard: [parity-ie] [parity-webkit])

Attachments

(5 attachments)

Reporter

Description

11 years ago
Build ID: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080930050845 Minefield/3.1b1pre

Bug 114649 fixed this issue partially, but it still does not trigger as often as IE and WebKit based browsers.

See bug 114649, comment 28 for a possible planned future solution.
Reporter

Updated

11 years ago
Whiteboard: [parity-ie] [parity-chrome] [parity-safari]

Comment 2

11 years ago
Posted file Visual Testcase

Comment 3

11 years ago
So it sounds like this depends on Bug 374980.
Reporter

Updated

11 years ago
Depends on: compositor
Summary: onresize events should fire more often while resizing the window → onresize events should fire at every resize while resizing the window, not just at 200ms intervals
Competition is good. I'm guessing this won't fit in 1.9.1, but it would be good to hear of any short-term mitigation options.

/be
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Assignee

Comment 6

11 years ago
Posted patch patchSplinter Review
This is not yet fully tested, but behavior and speed should be ok.
Fixes 'Visual Testcase', actually it behaves better than on webkit where resize
event isn't apparently fired often enough in some cases.
Based on that test Opera does *not* fire resize continuously.

I don't understand why firing resize event should depend on compositor.
Assignee

Comment 7

11 years ago
Especially I haven't tested how attachment 355212 [details] [diff] [review] works if page contains frames.
I think the rationale for depending on compositor is that we might be doing layout too often to begin with, whereas doing it at reasonably-spaced intervals might help keep the cost down of running script as well.  That said, we're probably ok just doing this.
Smaug: thanks!

Let's get this into 1.9.1 if at all possible.

/be
Flags: wanted1.9.1?
Assignee

Comment 11

11 years ago
Posted file a test for frames
Assignee

Comment 12

11 years ago
I made just as small changes as possible to the testcase, which expects
async resize.
Assignee: nobody → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #355285 - Flags: superreview?
Attachment #355285 - Flags: review?
Assignee

Updated

11 years ago
Attachment #355285 - Flags: superreview?(dbaron)
Attachment #355285 - Flags: superreview?
Attachment #355285 - Flags: review?(dbaron)
Attachment #355285 - Flags: review?
Does test_hover.html really need setTimeout(..., 100) rather that setTimeout(..., 0) ?
Assignee

Comment 14

11 years ago
with 100 it was more reliable if I accidentally moved mouse a bit.
I'd have expected the opposite.  Were you moving the mouse within the test document both times?  Either way, the test isn't going to work if you're moving the mouse, I don't think, so I'd prefer keeping it faster if possible.
And with test_hover.html, could you make sure that the test still fails if you comment out the FlushPendingNotifications call in PresShell::DispatchSynthMouseMove ?
Assignee

Comment 17

11 years ago
The test fails if PresShell::DispatchSynthMouseMove is commented out.
Comment on attachment 355285 [details] [diff] [review]
even simpler + needed changes to test_hover.html

OK, r+sr=dbaron, although I think I'd prefer the timeout in the test to be 0 rather than 100.
Attachment #355285 - Flags: superreview?(dbaron)
Attachment #355285 - Flags: superreview+
Attachment #355285 - Flags: review?(dbaron)
Attachment #355285 - Flags: review+
Attachment #355285 - Flags: approval1.9.1?
Assignee

Comment 19

11 years ago
http://hg.mozilla.org/mozilla-central/rev/b26e61de06dd
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Is there a followup bug for setTimeout 100 vs 0 not making the expected difference, or not being desirable in the test, or something?

/be

Comment 21

11 years ago
This fixed Windows bug 460620 (one more arument to have it on branch)

Updated

11 years ago
Blocks: 460620
This should go into 1.9.1 -- IMHO!

/be
Assignee

Comment 23

11 years ago
But not if there are regressions :( Bug 472730.
Depends on: 472730
What about just doing an async event instead of using a scriptrunner?  So just post the runnable into the event queue?
Assignee

Comment 25

11 years ago
Yeah, need to try that.
Assignee

Comment 26

11 years ago
Unfortunately that doesn't work.
Version: unspecified → Trunk
Flags: wanted1.9.1? → wanted1.9.1+
What's the performance impact when resizing a window here? Measurable?
Assignee

Updated

11 years ago
Depends on: 473805

Updated

11 years ago
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Flags: blocking1.9.1?
The regression is fixed, right? Any reason not to take this on 1.9.1?
smaug, do you think we should wait on bug 473805 to land this on 1.9.1?
Assignee

Comment 31

11 years ago
Yes.
Comment on attachment 355285 [details] [diff] [review]
even simpler + needed changes to test_hover.html

Clearing nom per Smaug's comment. Please renominate once bug 473805 has landed.
Attachment #355285 - Flags: approval1.9.1?
Whiteboard: [parity-ie] [parity-chrome] [parity-safari] → [parity-ie] [parity-chrome] [parity-safari][needs 191 approval after bug 473805]
Flags: wanted1.9.1.x?
status1.9.1: --- → ?
Flags: wanted1.9.1.x?
Whiteboard: [parity-ie] [parity-chrome] [parity-safari][needs 191 approval after bug 473805] → [parity-ie] [parity-chrome] [parity-safari]
Whiteboard: [parity-ie] [parity-chrome] [parity-safari] → [parity-ie] [parity-webkit]
You need to log in before you can comment on or make changes to this bug.