Closed
Bug 748048
Opened 13 years ago
Closed 13 years ago
Text search input box doesn't show/update entered text, but it will get added and submitted
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox14 fixed, blocking-fennec1.0 beta+)
VERIFIED
FIXED
Firefox 15
People
(Reporter: aryx, Assigned: ajuma)
References
Details
(Whiteboard: [has reviewed patch])
Attachments
(3 files, 3 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
roc
:
review+
blassey
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ajuma
:
review+
blassey
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Fennec native trunk 2012-04-23, Android 4.0.4 (stock), Google Nexus S
On http://www.heise.de/preisvergleich/?o=53 , tapping onto the input/search box next to "Artikelsuche:" and typing something won't update the content. If you submit using the "Go" button from the software keyboard, you can see that the browser received the text you entered.
I can produce something similar:
1. go to http://people.mozilla.com/~nhirata/html_tp/NextAction_Bug614356.html
2. click in the First name within the "Two input elements (should have next)"
3. type something and hit the next button on the vkb
4. try to type something in the Last name
Expected: you can see what you typed in the Last name field
Actual: you can't see anything unless you invalidate the field (ie scroll the field out of sight and bring it back)
Updated•13 years ago
|
blocking-fennec1.0: --- → ?
Comment 2•13 years ago
|
||
nhirata, which phone are you testing? I can't reproduce with Nightly 2012-04-20 or 2012-04-24 on my Galaxy Nexus. (I don't have a Nexus S handy.)
Does the typed text appear if you zoom the page in and out (forcing Fennec to redraw the screen)?
LG Revolution. I can show you in person.
Updated•13 years ago
|
Assignee: nobody → ajuma
blocking-fennec1.0: ? → beta+
Assignee | ||
Comment 5•13 years ago
|
||
Frame tree invalidation seems to be proceeding correctly when text is being entered, and this is (correctly) resulting in a call to FrameLayerBuilder::InvalidateThebesLayerContents. However, when nsViewManager::InvalidateWidgetArea gets called, the intersection of aWidgetView->GetInvalidationDimensions() and aDamagedRegion is empty, since aDamagedRegion turns out to be below aWidgetView-GetInvalidationDimensions() (it could be we're failing to adjust for the presence of the keyboard in calculating one of these regions) so nsWindow::Invalidate() doesn't get called.
As a quick workaround, we can just add a call to nsWindow::Invalidate when handling IME_EVENTs and KEY_EVENTs in nsWindow::OnGlobalAndroidEvent, but this of course won't fix other invalidation issues we might be having while the keyboard is shown.
I guess this is because of mobile's view transforms meaning that layout doesn't actually know the correct screen area for the content that's being invalidated.
Just take out
// If the bounds don't overlap at all, there's nothing to do
nsRegion intersection;
intersection.And(aWidgetView->GetInvalidationDimensions(), aDamagedRegion);
if (intersection.IsEmpty()) {
return;
}
and use aDamagedRegion instead of 'intersection'. Shouldn't cause problems.
Assignee | ||
Comment 9•13 years ago
|
||
This implements Roc's suggestion from Comment 7. It turns out there are also a couple places in nsWindow where we intersect the damaged region with the widget's bounds; these also produce empty intersections and prevent us from painting, so they need to be removed as well.
Attachment #619616 -
Flags: review?(roc)
Comment 10•13 years ago
|
||
Comment on attachment 619616 [details] [diff] [review]
Don't restrict invalidation to the widget's boundaries.
Didn't we add nsIView::SetInvalidationDimensions in bug 593243 to solve a similar problem for XUL fennec? Can we just use that instead?
Assignee | ||
Comment 11•13 years ago
|
||
(In reply to Timothy Nikkel (:tn) from comment #10)
> Comment on attachment 619616 [details] [diff] [review]
> Don't restrict invalidation to the widget's boundaries.
>
> Didn't we add nsIView::SetInvalidationDimensions in bug 593243 to solve a
> similar problem for XUL fennec? Can we just use that instead?
Perhaps.
Right now, we're never calling SetInvalidationDimensions, since in nsDOMWindowUtils::SetDisplayPortForElement, presContext->IsRoot() is false. However, even if we make the call unconditionally, mHaveInvalidationDimensions is false for aWidgetView in InvalidateWidgetArea. On which view do we really want to call SetInvalidationDimensions in SetDisplayPortForElement?
(In reply to Timothy Nikkel (:tn) from comment #10)
> Didn't we add nsIView::SetInvalidationDimensions in bug 593243 to solve a
> similar problem for XUL fennec? Can we just use that instead?
Yes, however, it seems like it might be simpler just to remove these checks (and SetInvalidationDimensions).
Note that when DLBI lands we'll automatically limit invalidations to whatever's visible in the displayport.
(In reply to Ali Juma [:ajuma] from comment #11)
> Right now, we're never calling SetInvalidationDimensions, since in
> nsDOMWindowUtils::SetDisplayPortForElement, presContext->IsRoot() is false.
> However, even if we make the call unconditionally,
> mHaveInvalidationDimensions is false for aWidgetView in
> InvalidateWidgetArea. On which view do we really want to call
> SetInvalidationDimensions in SetDisplayPortForElement?
You'd need to set it on the root view for the root chrome document. That's annoying.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> Yes, however, it seems like it might be simpler just to remove these checks
> (and SetInvalidationDimensions).
Ali, can you write a patch that does that?
I guess the thing to worry about is invalidates outside the displayport triggering repainting before DLBI lands. Currently nsGfxScrollFrame::InvalidateInternal trims the damage rect to the displayport but I don't see any early exits when the damage rect is empty.
Seems to me that nsIFrame::InvalidateInternalAfterResize should check whether aDamageRect is empty and bail out if it is.
Assignee | ||
Comment 16•13 years ago
|
||
This removes {Set,Get}InvalidationDimensions, and makes nsIFrame::InvalidateInternalAfterResize bail out when aDamageRect is empty.
Attachment #619616 -
Attachment is obsolete: true
Attachment #619616 -
Flags: review?(roc)
Attachment #619944 -
Flags: review?(roc)
Assignee | ||
Comment 17•13 years ago
|
||
This removes the intersections between the damaged region and the widget's bounds that are happening in nsWindow.
Attachment #619946 -
Flags: review?(roc)
Assignee | ||
Comment 18•13 years ago
|
||
Uploading the correct version this time.
Attachment #619946 -
Attachment is obsolete: true
Attachment #619946 -
Flags: review?(roc)
Attachment #619947 -
Flags: review?(roc)
Comment on attachment 619944 [details] [diff] [review]
Part 1: Remove SetInvalidationDimensions and GetInvalidationDimensions.
Review of attachment 619944 [details] [diff] [review]:
-----------------------------------------------------------------
::: view/public/nsIView.h
@@ -175,5 @@
> - *
> - * The caller is responsible for invalidating the area that may lie
> - * outside the view dimensions but inside |aRect| after this call.
> - */
> - void SetInvalidationDimensions(const nsRect* aRect);
Change UUID on nsIView
Attachment #619944 -
Flags: review?(roc) → review+
Attachment #619947 -
Flags: review?(roc) → review+
Updated•13 years ago
|
Whiteboard: [has reviewed patch]
Assignee | ||
Comment 20•13 years ago
|
||
Patch with nsIView's UUID changed. Carrying forward r=roc.
Attachment #619944 -
Attachment is obsolete: true
Attachment #620023 -
Flags: review+
Assignee | ||
Comment 21•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3e391f4c1bcb
https://hg.mozilla.org/mozilla-central/rev/5be71aa88a16
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
Assignee | ||
Comment 22•13 years ago
|
||
Comment on attachment 620023 [details] [diff] [review]
Part 1: Remove SetInvalidationDimensions and GetInvalidationDimensions.
[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: In many situations, entered text won't appear until panning/zooming occurs or until the keyboard is hidden.
Testing completed (on m-c, etc.): Just landed on m-c.
Risk to taking this patch (and alternatives if risky): Seems low-risk, but note that this patch also affects desktop builds.
String changes made by this patch: None
Attachment #620023 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 23•13 years ago
|
||
Comment on attachment 619947 [details] [diff] [review]
Bug 748048 - Part 2: Don't restrict invalidation to the widget's boundaries on Android.
[Approval Request Comment]
See above.
Attachment #619947 -
Flags: approval-mozilla-aurora?
Updated•13 years ago
|
Attachment #619947 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•13 years ago
|
Attachment #620023 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 24•13 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/7473683e80d2
https://hg.mozilla.org/releases/mozilla-aurora/rev/3bf385993480
status-firefox14:
--- → fixed
Comment 25•13 years ago
|
||
Verified on:
Nightly 15.0a1 2012-04-07 / Aurora 14.0a2 2012-04-07
Device: HTC Desire / Samsung Galaxy S2
OS: Android 2.2 / Android 2.3.4
Unable to reproduce the issue using http://www.heise.de/preisvergleich/?o=53 or http://people.mozilla.com/~nhirata/html_tp/NextAction_Bug614356.html. Also I was unable to reproduce the issue from the duplicate bugs: Bug 737008 and Bug 750132.
Status: RESOLVED → VERIFIED
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•