Closed Bug 259636 Opened 15 years ago Closed 10 years ago

Should setting .value on text inputs flush reflows?

Categories

(Core :: Layout: Form Controls, defect, P2)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(3 files)

Right now, setting .value on a text input sets the value and flushes reflows
(instead of just posting a reflow event).

Editor does this so that things updated immediately when typing.  But for
setting .value, I don't think that's desirable.
Comment on attachment 159010 [details] [diff] [review]
Perhaps like this

Can anyone see a reason this would be a bad idea?
Attachment #159010 - Flags: superreview?(dbaron)
Attachment #159010 - Flags: review?(brade)
Hey bz, as I mentioned on IRC, checkout bug 174823 and bug 141900 for some
background on the problems with async reflow.

How does your patch act on a page where there's an event handler (filter) that
also sets the value of the text widget?
Attachment #159010 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 159010 [details] [diff] [review]
Perhaps like this

temporary r- until there is a response to Kin's comments
Attachment #159010 - Flags: review?(brade) → review-
Blocks: 83841
The testcase seems to work OK over here in a patched build on Linux, but maybe I
just type too slowly...

My build _does_ show bug 165130, and I would fully expect that bug 158782 and
bug 151882 would show up when the filter is being used (though I can't really
reproduce them, as I said).

It sounds like we should make bug 174823 somewhat more of a priority...  Should
this one be marked dup of it?  This is a more modest proposal, but it seems like
it has all the same issues.
Depends on: 174823
Depends on: 262760
Attachment #159010 - Flags: review- → review?(kinmoz)
Comment on attachment 159010 [details] [diff] [review]
Perhaps like this

Kin, with bug 244366 fixed the various testcases no longer flicker with this
patch....
Priority: -- → P2
Summary: Should setting .value on text inputs flush reflows? → [FIX]Should setting .value on text inputs flush reflows?
Target Milestone: --- → mozilla1.8beta
Comment on attachment 159010 [details] [diff] [review]
Perhaps like this

Talked to kin some more.  There are also caret-painting issues, and since that
does not go through the viewmanager those are not resolved yet... (to be
precise, deleting content leaves the caret no place to go so it flickers to
beginning of line).
Attachment #159010 - Flags: review?(kinmoz) → review-
Summary: [FIX]Should setting .value on text inputs flush reflows? → Should setting .value on text inputs flush reflows?
The patch for Bug 518122 does the same what this does.
When using the testcase in this bug, the patch for bug 518122 and bug 518950
fixes the flickering I otherwise see on trunk builds. (At least the spellcheck markers flickers.)

But I need to test something else; perhaps something like reversing the user input
in keypress handler. That would prevent the value cache the patch for bug 518122
adds.
Blocks: 518122
Attached file reverse textarea
Works well with the patch.
Sounds good.  If things look good in your testing, let's get this landed!
Comment on attachment 159010 [details] [diff] [review]
Perhaps like this

Since smaug says this behaves well...
Attachment #159010 - Flags: review- → review?(roc)
Pushed http://hg.mozilla.org/mozilla-central/rev/678e3c57654a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.8beta1 → mozilla1.9.3a1
Depends on: 535212
You need to log in before you can comment on or make changes to this bug.