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

RESOLVED FIXED

Status

()

RESOLVED FIXED
10 years ago
5 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

10 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)

Comment 1

10 years ago
Created attachment 341107 [details]
Testcase showing how often onresize fires
(Reporter)

Updated

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

Comment 2

10 years ago
Created attachment 341112 [details]
Visual Testcase

Comment 3

10 years ago
So it sounds like this depends on Bug 374980.
(Reporter)

Updated

10 years ago
Depends on: 374980
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
Duplicate of this bug: 387917
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

10 years ago
Created attachment 355212 [details] [diff] [review]
patch

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

10 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

10 years ago
Created attachment 355244 [details]
a test for frames
(Assignee)

Comment 12

10 years ago
Created attachment 355285 [details] [diff] [review]
even simpler + needed changes to test_hover.html

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

10 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

10 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

10 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+

Updated

10 years ago
Attachment #355285 - Flags: approval1.9.1?
(Assignee)

Comment 19

10 years ago
http://hg.mozilla.org/mozilla-central/rev/b26e61de06dd
Status: ASSIGNED → RESOLVED
Last Resolved: 10 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

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

Updated

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

/be
(Assignee)

Comment 23

10 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

10 years ago
Yeah, need to try that.
(Assignee)

Comment 26

10 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

10 years ago
Depends on: 473805

Updated

10 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

10 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?
status1.9.1: ? → wontfix
Whiteboard: [parity-ie] [parity-chrome] [parity-safari][needs 191 approval after bug 473805] → [parity-ie] [parity-chrome] [parity-safari]

Updated

9 years ago
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.