Closed Bug 457862 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.1 --- wontfix

People

(Reporter: bugzilla, Assigned: smaug)

References

(Depends on 1 open bug)

Details

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

Attachments

(5 files)

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.
Whiteboard: [parity-ie] [parity-chrome] [parity-safari]
Attached file Visual Testcase
So it sounds like this depends on Bug 374980.
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
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?
Attached 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.
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?
Attached file a test for frames
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?
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) ?
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 ?
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?
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
This fixed Windows bug 460620 (one more arument to have it on branch)
Blocks: 460620
This should go into 1.9.1 -- IMHO!

/be
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?
Yeah, need to try that.
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?
Depends on: 473805
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?
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.