777260 - Text not rendered in <input> until switching tabs or otherwise re-rendering the window

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Attachment: screencast showing the misplaced autocompletion dropdown and text rendering issues

(Sorry if that's not the right component.)

We use the provided URL to comment on bugs that have been merged and this bug has been around for quite some time but I always forgot to file it.

STR:

1) Open http://www.graememcc.co.uk/m-cmerge/?cset=ef20925bc2a5
2) Click the "Next" button in the upper right
3) Scroll to the bottom and click "Submit the changes above"
4) Press a key to enter a value into the "Username" field.

Now there's not even text showing up in the input field. If you entered your credentials before, the autocompletion popup will be misplaced and positioned in the lower right.

If you enter text or select an entry from the autocompletion dropdown, it's first rendered when switching to a different tab and back to the current one or doing a similar action.

I attached a screencast of this issue. I checked with (I think) RyanVM on IRC and he experiences the same issue, so it's not only me :)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Ok, the text not being rendered seems to be a separate issue with the regression range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=82b6c5885345&tochange=3613cbdc3481

Most probably caused by bug 539356. Will file another bug about the misplaced autocompletion dropdown.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Requesting tracking for this regression, like all regressions.  ;)

Comment 3

[Matt Woodrow (:mattwoodrow)]

This is fixed by the patch in bug 777617

Comment 4

[Ryan VanderMeulen [:RyanVM]]

I'm still seeing this on m-cMerge on a build from m-c tip as of this morning.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking this regression, esp. since comment 4 suggests that bug 777617 didn't fix the issue.

Comment 6

[Matt Woodrow (:mattwoodrow)]

The regression range for this seems to be incorrect, I can still reproduce the issue with rev 82b6c5885345.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Nightly bisection gives me http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5cdbeae14405&tochange=bf8f2961d0cc

Bug 758620 maybe?

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Might be worth seeing what this looks like after bug 779269 is fixed.

Comment 9

[Chris Lord [:cwiiis]]

The patch I currently have in bug 779269 doesn't fix this issue. The invalidation problem may well be related to bug 758620, but I think the offset problem is pre-existing and likely something else.

Comment 10

[Chris Lord [:cwiiis]]

Loathe as I am to give myself more layout work, this is likely my fault. Looking into it.

Comment 11

[Chris Lord [:cwiiis]]

The problem here seems to be that the display-list ends up as:

FixedPosition
  nsDisplayTransform
    EverythingElse

FixedPosition and nsDisplayTransform are both container layers for the same frame, but nsDisplayTransform isn't active, so the invalidation gets stopped and EverythingElse doesn't get any invalidation.

This feels like it could happen under quite a few other circumstances too...?

Anyway, still tracing, should be able to come up with a patch soon I hope.

Comment 12

[Chris Lord [:cwiiis]]

Hah, the problem is actually much sillier and simpler than that, thankfully :) Patch incoming, assuming what I just spotted is indeed the problem.

Comment 13

[Chris Lord [:cwiiis]]

Ok, it wasn't the mistake that I spotted, but I'm pretty certain that's a bug too, so will attach that separately. Still think I'm near to a fix.

Comment 14

[Chris Lord [:cwiiis]]

The problem is as I stated in comment #11, I'm just trying to find a sensible way to force the nsDisplayTransform to be active in this situation. Alternatively, we could special-case transforms in FrameLayerBuilder...

Comment 15

[Chris Lord [:cwiiis]]

Attachment: ChildrenCanBeInactive ignores LAYER_ACTIVE_FORCE

This seems like a bug to me, but perhaps I misunderstand what this is trying to do?

Comment 16

[Chris Lord [:cwiiis]]

I think a nice way of fixing this would be to pass the container frame in GetLayerState, then let the display item decide what to return based on that - then nsDisplayTransform could return active when it knows that it's the container and needs to build a layer.

Comment 17

[Chris Lord [:cwiiis]]

Attachment: Pass the container frame in GetLayerState, use it in nsDisplayTransform

This fixes the problem in this bug - there may be other items that should do something in this case too(?)

I don't like that there are two GetLayerState's, but I'm attaching this smaller patch to get feedback first.

If this is ok, feel free to r+ of course, but I think that if this is the method we go for to fix this, we should change the function prototype for GetLayerState, add a default and change all the overriding prototypes too.

Let me know your thoughts!

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 650624 [details] [diff] [review]
ChildrenCanBeInactive ignores LAYER_ACTIVE_FORCE

Review of attachment 650624 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch!

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 650632 [details] [diff] [review]
Pass the container frame in GetLayerState, use it in nsDisplayTransform

Review of attachment 650632 [details] [diff] [review]:
-----------------------------------------------------------------

OK, so there are two display items for the same frame that create ContainerLayers and the inner one is inactive.

I think the problem is due to this comment in nsIFrame::InvalidateInternalAfterResize:
    // XXX for now I'm going to assume this is in the local coordinate space
    // This only matters for frames with transforms and retained layers,
    // which can't happen right now since transforms trigger fallback
    // rendering and the display items that trigger layers are nested inside
    // the nsDisplayTransform
This comment is no longer true, since the nsDisplayTransform is not the outermost item. So I guess we're calling FrameLayerBuilder::InvalidateThebesLayerContents OK but with the wrong area, and the bug is that in this case we would have needed to pass the transformed area.

Assuming that's true, then I think the correct way to fix this is in FrameLayerBuilder. We can define InvalidateThebesLayerContents(frame) to always invalidate coordinates *before* the frame's transforms are applied. Then in FrameLayerBuilder::BuildContainerLayerFor, if aContainerFrame->HasTransform() and aContainerItem's display item subtree contains the nsDisplayTransform, then we should transform the invalid region (just its bounds would be fine) before applying it. Make sense?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Needs moar tests, btw ;)

Comment 21

[Chris Lord [:cwiiis]]

Attachment: Fix transformed inactive layers in container layers

Do as suggested in comment #19, fixes problem :)

Comment 22

[Chris Lord [:cwiiis]]

(In reply to Ryan VanderMeulen from comment #20)
> Needs moar tests, btw ;)

Agreed, will try to come up with a test too.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 650839 [details] [diff] [review]
Fix transformed inactive layers in container layers

Review of attachment 650839 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/FrameLayerBuilder.cpp
@@ +2318,5 @@
> +        // If the container frame has a transform and it's contained in the
> +        // container item's sub-tree, we need to transform the invalid region
> +        // before applying it.
> +        if (aChildren.GetTop()->GetType() == nsDisplayItem::TYPE_TRANSFORM) {
> +          transformItem = static_cast<nsDisplayTransform*>(aChildren.GetTop());

This is pretty good but really you should search the list recursively --- in the future we might add other items so it might not be the direct child. It's probably also not good to assume it's the only child.

Also, you should check that it's for aContainerFrame.

Comment 24

[Chris Lord [:cwiiis]]

Attachment: Fix transformed inactive layers in container layers v2

This hopefully addresses comment #23.

Comment 25

[Chris Lord [:cwiiis]]

Attachment: Fix transformed inactive layers in container layers v3

This fixes a crash due to lack of a null check on the display-item child list, and a warning from using the merged frame instead of the container frame when doing the transform when applying invalidation to merged frames.

Comment 26

[Chris Lord [:cwiiis]]

I've crafted a page that demonstrates this bug pretty easily at http://chrislord.net/files/mozilla/non-root-transform.html

When it works, you see 'Hello World' in the input box, when it fails, just 'Hello' (and switching tabs or forcing it to invalidate some other way will make it update).

Just seeing how best to turn this into a test. The two input changes seem necessary, I assume because the first one makes some layers active, or clears some state somewhere. If the timeout is very short, it occasionally works, but this may be due to redraws after loading.

This can probably be achieved as a reftest-wait.

Comment 27

[Chris Lord [:cwiiis]]

Not been able to get this to work as a reftest... Either it fails both before and after the patch, or it passes on both - crafting it exactly as I have it on the page in comment #26, it just passes on both.

May have to leave this for now. If I don't get it working, I'll file a bug so we don't forget it.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

MozReftestInvalidate?

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 650926 [details] [diff] [review]
Fix transformed inactive layers in container layers v3

Review of attachment 650926 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/FrameLayerBuilder.cpp
@@ +2314,5 @@
>  
>      if (aManager == mRetainingManager) {
> +      nsDisplayTransform* transformItem = nsnull;
> +      if (aContainerFrame->IsTransformed() &&
> +          aContainerItem->GetType() != nsDisplayItem::TYPE_TRANSFORM) {

Probably should test this with preserve-3d wrapped in position:fixed.

Move this search out to a separate function.

@@ +2322,5 @@
> +        nsTArray<nsDisplayItem*> queue;
> +        queue.AppendElement(aContainerItem);
> +        while (queue.Length()) {
> +          nsDisplayItem* item = queue[0];
> +          queue.RemoveElementAt(0);

Remove from the end

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also I think this can be written so that we don't copy all the child items into an array, just the ones that have the same frame as aContainerFrame.

Comment 31

[Chris Lord [:cwiiis]]

Attachment: Fix transformed inactive layers in container layers v4

This splits it out into a separate function, removes from the end and only recurses through children with the same underlying frame.

I did some messing around with some more complex transforms and preserve-3d, but I couldn't craft a case that used preserve-3d and breaks in current Nightly, so I'm not sure how best to test that.

Comment 32

[Chris Lord [:cwiiis]]

Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcea5d24a1b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fe1f9765d81

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fcea5d24a1b4
https://hg.mozilla.org/mozilla-central/rev/9fe1f9765d81

Comment 34

[Dennis Jakobsen]

Not seeing selection of the "Hello World" text in the input field is another bug then?

Comment 35

[Chris Lord [:cwiiis]]

(In reply to Dennis Jakobsen from comment #34)
> Not seeing selection of the "Hello World" text in the input field is another
> bug then?

This is the same bug - you shouldn't see this with this patch applied... Do you?

Comment 36

[Dennis Jakobsen]

I just checked the latest mozilla-inbound build from tinderbox and it works correctly now. I was using regular Nightly before, apologies.

Comment 37

[Chris Lord [:cwiiis]]

Comment on attachment 651428 [details] [diff] [review]
Fix transformed inactive layers in container layers v4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Invalidations are lost for transformed children of position:fixed elements. This includes typing/focus rings/etc.
User impact if declined: Typing in some fields would appear to have no effect
Testing completed (on m-c, etc.): Tested locally and been on m-c for a day with no reports of terrible regressions, yet :)
Risk to taking this patch (and alternatives if risky): Probably mobile-only, nsDisplayFixedPosition is disabled for desktop in aurora. Medium risk of causing different invalidation issues for transformed elements in fixed-position elements.
String or UUID changes made by this patch: None

Comment 38

[Lukas Blakk [:lsblakk] use ?needinfo]

Why are you requesting uplift for this? What's the population of people who are going to hit this in Aurora and is this affecting desktop users in Aurora?

Comment 39

[Chris Lord [:cwiiis]]

(In reply to Lukas Blakk [:lsblakk] from comment #38)
> Why are you requesting uplift for this? What's the population of people who
> are going to hit this in Aurora and is this affecting desktop users in
> Aurora?

This is mobile only. I'm requesting uplift as it could result in very frustrating problems when browsing (e.g. input fields not updating when you type in them).

Comment 40

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 651428 [details] [diff] [review]
Fix transformed inactive layers in container layers v4

Mobile only, looks good for Aurora then, thanks for the clarification.

Comment 41

[Chris Lord [:cwiiis]]

Pushed to aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/b49f8ce5b0f2

Comment 42

[Ryan VanderMeulen [:RyanVM]]

(In reply to Tim Taubert [:ttaubert] (on vacation, back Sep 5th) from comment #1)
> Most probably caused by bug 539356. Will file another bug about the
> misplaced autocompletion dropdown.

Did this bug ever get filed? It's definitely still happening on m-cMerge.

Comment 43

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Ryan VanderMeulen from comment #42)
> (In reply to Tim Taubert [:ttaubert] (on vacation, back Sep 5th) from
> comment #1)
> > Most probably caused by bug 539356. Will file another bug about the
> > misplaced autocompletion dropdown.
> 
> Did this bug ever get filed? It's definitely still happening on m-cMerge.

Yes, I filed bug 777263 which then got duped to bug 467442.

Comment 44

[Ioana (away)]

I tried to verify this issue, but I couldn't reproduce it on pre-fix builds. Was this issue intermittent?

Tim, are there any details for reproducing this bug that you haven't specified in comment 0?

Comment 45

[Tim Taubert [:ttaubert] (inactive)]

I'm pretty sure the STR is up-to-date as that's the same for bug 794686 - which is a regression of this bug. So you might want to wait before verifying :)

Comment 46

[Ioana (away)]

Thanks, Tim. Bug 794686 will get verified then when fixed.