Closed Bug 748048 Opened 8 years ago Closed 8 years ago

Text search input box doesn't show/update entered text, but it will get added and submitted

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 15
Tracking Status
firefox14 --- fixed
blocking-fennec1.0 --- beta+

People

(Reporter: aryx, Assigned: ajuma)

References

Details

(Whiteboard: [has reviewed patch])

Attachments

(3 files, 3 obsolete files)

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)
blocking-fennec1.0: --- → ?
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.
Assignee: nobody → ajuma
blocking-fennec1.0: ? → beta+
Duplicate of this bug: 737008
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.
Attached patch WorkaroundSplinter Review
This implements the workaround described in Comment 5.
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.
Duplicate of this bug: 750132
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 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?
(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.
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)
This removes the intersections between the damaged region and the widget's bounds that are happening in nsWindow.
Attachment #619946 - Flags: review?(roc)
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+
Whiteboard: [has reviewed patch]
Patch with nsIView's UUID changed. Carrying forward r=roc.
Attachment #619944 - Attachment is obsolete: true
Attachment #620023 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/3e391f4c1bcb
https://hg.mozilla.org/mozilla-central/rev/5be71aa88a16
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
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?
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?
Attachment #619947 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #620023 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
You need to log in before you can comment on or make changes to this bug.