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

RESOLVED FIXED in Firefox 16

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: cwiiis)

Tracking

(Depends on: 1 bug, {regression})

Trunk
mozilla17
x86_64
Linux
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox16 fixed, firefox17+ fixed)

Details

(Whiteboard: [qa-], URL)

Attachments

(3 attachments, 4 obsolete attachments)

Created attachment 645676 [details]
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 :)
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.
Blocks: 539356
Keywords: regression
Summary: Misplaced autocompletion dropdown and text not rendered in <input> → Text not rendered in <input> until switching tabs or otherwise re-rendering the window
Requesting tracking for this regression, like all regressions.  ;)
tracking-firefox17: --- → ?
This is fixed by the patch in bug 777617
I'm still seeing this on m-cMerge on a build from m-c tip as of this morning.
Tracking this regression, esp. since comment 4 suggests that bug 777617 didn't fix the issue.
status-firefox17: --- → affected
tracking-firefox17: ? → +
The regression range for this seems to be incorrect, I can still reproduce the issue with rev 82b6c5885345.
Nightly bisection gives me http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5cdbeae14405&tochange=bf8f2961d0cc

Bug 758620 maybe?
Might be worth seeing what this looks like after bug 779269 is fixed.
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
Loathe as I am to give myself more layout work, this is likely my fault. Looking into it.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
(Assignee)

Comment 11

5 years ago
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.
(Assignee)

Comment 12

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

Comment 13

5 years ago
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.
(Assignee)

Comment 14

5 years ago
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...
(Assignee)

Comment 15

5 years ago
Created attachment 650624 [details] [diff] [review]
ChildrenCanBeInactive ignores LAYER_ACTIVE_FORCE

This seems like a bug to me, but perhaps I misunderstand what this is trying to do?
Attachment #650624 - Flags: feedback?(roc)
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Comment 17

5 years ago
Created attachment 650632 [details] [diff] [review]
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!
Attachment #650632 - Flags: feedback?(roc)
Comment on attachment 650624 [details] [diff] [review]
ChildrenCanBeInactive ignores LAYER_ACTIVE_FORCE

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

Good catch!
Attachment #650624 - Flags: review+
Attachment #650624 - Flags: feedback?(roc)
Attachment #650624 - Flags: feedback+
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?
Needs moar tests, btw ;)
(Assignee)

Comment 21

5 years ago
Created attachment 650839 [details] [diff] [review]
Fix transformed inactive layers in container layers

Do as suggested in comment #19, fixes problem :)
Attachment #650632 - Attachment is obsolete: true
Attachment #650632 - Flags: feedback?(roc)
Attachment #650839 - Flags: review?(roc)
(Assignee)

Comment 22

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

Agreed, will try to come up with a test too.
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.
(Assignee)

Comment 24

5 years ago
Created attachment 650897 [details] [diff] [review]
Fix transformed inactive layers in container layers v2

This hopefully addresses comment #23.
Attachment #650839 - Attachment is obsolete: true
Attachment #650839 - Flags: review?(roc)
Attachment #650897 - Flags: review?(roc)
(Assignee)

Comment 25

5 years ago
Created attachment 650926 [details] [diff] [review]
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.
Attachment #650897 - Attachment is obsolete: true
Attachment #650897 - Flags: review?(roc)
Attachment #650926 - Flags: review?(roc)
(Assignee)

Comment 26

5 years ago
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.
(Assignee)

Comment 27

5 years ago
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.
MozReftestInvalidate?
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
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.
(Assignee)

Comment 31

5 years ago
Created attachment 651428 [details] [diff] [review]
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.
Attachment #650926 - Attachment is obsolete: true
Attachment #650926 - Flags: review?(roc)
Attachment #651428 - Flags: review?(roc)
Attachment #651428 - Flags: review?(roc) → review+
(Assignee)

Comment 32

5 years ago
Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcea5d24a1b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fe1f9765d81
https://hg.mozilla.org/mozilla-central/rev/fcea5d24a1b4
https://hg.mozilla.org/mozilla-central/rev/9fe1f9765d81
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Comment 34

5 years ago
Not seeing selection of the "Hello World" text in the input field is another bug then?
(Assignee)

Comment 35

5 years ago
(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

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

Comment 37

5 years ago
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
Attachment #651428 - Flags: approval-mozilla-aurora?
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?
(Assignee)

Comment 39

5 years ago
(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).

Updated

5 years ago
status-firefox17: affected → fixed
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.
Attachment #651428 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 41

5 years ago
Pushed to aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/b49f8ce5b0f2
(Assignee)

Updated

5 years ago
status-firefox16: --- → fixed
(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.

Updated

5 years ago
Depends on: 787471
(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.
Keywords: verifyme

Comment 44

5 years ago
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?
Whiteboard: [qa?]
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

5 years ago
Thanks, Tim. Bug 794686 will get verified then when fixed.
Keywords: verifyme
Whiteboard: [qa?] → [qa-]

Updated

5 years ago
Depends on: 815270
You need to log in before you can comment on or make changes to this bug.