Closed Bug 1874766 Opened 1 month ago Closed 1 month ago

[KDE/wayland] Window rules to position PiP window broke after bug 1838045

Categories

(Toolkit :: Picture-in-Picture, defect, P3)

defect

Tracking

()

RESOLVED FIXED
123 Branch
Tracking Status
firefox-esr115 --- wontfix
firefox121 --- wontfix
firefox122 --- wontfix
firefox123 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(3 files)

As described in bug 1767414 comment 3 and co, it seems to be a regression. I can repro with a rule like the attached one.

The first window gets positioned correctly. The next doesn't. Given the regressing bug, it's likely because:

  • The window is shown right after the window load event.
  • Bug 1767414 affects the load event timing for cached stylesheets, probably making the window show up sooner.
  • Probably, the PiP window tries to resize itself, now after the load event, resetting the position.

I'm not sure (yet) whether this is a bug in the style system, or just something that the PiP code happened to rely on.

It might be that KWin shouldn't reset the window position for wayland windows like this... Vlad, do you have any thoughts there?

Flags: needinfo?(vlad.zahorodnii)

Set release status flags based on info from the regressing bug 1838045

Does "Force" rule work? I suspect that the title might have been set after the initial surface commit. Remember and Apply Initially apply only when the window is unmapped, so it's important that kwin can identify windows when the pip surface is committed for the first time.

I can confirm that Apply Initially doesn't work

Flags: needinfo?(vlad.zahorodnii)

Yes, "Force" does work. Yeah looks like setting the title is async, so my patch which made the load fire faster broke this in a sense.

Flags: needinfo?(emilio)

:emilio I suppose you don't need my window rules anymore now that you've managed to reproduce it.

Great to see some progress being made to fix this! Specially now that Firefox uses Wayland by default and window rules is the only way to manage the PiP window.

Severity: -- → S3
Priority: -- → P3
Flags: needinfo?(emilio)
Assignee: nobody → emilio
Status: NEW → ASSIGNED
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/830a664759c7
Make title change events block onload in chrome docshells. r=smaug

Hiro / botond, do you know what that assertion means and why delaying the load event a bit breaks it? Bug 1824886 comment 23 and following have a similar example.

It seems the assertion is harmless tho, as we are just overriding the previous zoom constraints (https://searchfox.org/mozilla-central/rev/1aa61dcd48e128a8cbfbe59b7ba43d31bd3c248a/widget/nsBaseWidget.cpp#1056-1068)

Flags: needinfo?(hikezoe.birchill)
Flags: needinfo?(botond)
Flags: needinfo?(emilio)

It doesn't hold if the load event is delayed a bit, but it breaking
shouldn't change any behavior / shouldn't matter in practice.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)

Hiro / botond, do you know what that assertion means and why delaying the load event a bit breaks it?

I think the idea behind APZ including the pres shell id in the key that it uses to look up objects like ZoomConstraints is that this is how it detects e.g. a new document being loaded which might have different zoom constraints.

I'm a bit fuzzy on what other conditions can cause the pres shell to be reconstructed (and therefore the pres shell id to be updated), but as mentioned in the review, looking at how this value is used later on, the change should be harmless.

Flags: needinfo?(botond)

I am not so familiar with the presshellID's restriction in the ZoomConstraints, so I am debugging what's going on there with D198684. Anyways, in short the assertion happens "about:blank" -> "about:blank" transition for some reasons, and more over the ViewID is also different so D198785 will not solve.

I will have a meeting right now, I will debug it further later.

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6e3e7f97caf3
Relax assertion in nsBaseWidget::UpdateZoomConstraints. r=botond

Okay, for what's it's worth I am going to comment what I've found.

So it looks that the assertions are to make sure each valid ZoomConstrains in nsBaseWidget is reset before being updated with the new one. So for example, without D198684 nsBaseWidget::UpdateZoomConstraints gets called in the following order;

  1. UpdateZoomConstraints(aPresShellId = 13, aViewId = 7, a valid ZoomConstraints)
  2. UpdateZoomConstraints(aPresShellId = 13, aViewId = 7, Nothing())
  3. UpdateZoomConstraints(aPresShellId = 14, aViewId = 8, a valid ZoomConstraints)

But with D198684;

  1. UpdateZoomConstraints(aPresShellId = 13, aViewId = 7, a valid ZoomConstraints)
  2. UpdateZoomConstraints(aPresShellId = 14, aViewId = 8, a valid ZoomConstraints)
  3. UpdateZoomConstraints(aPresShellId = 13, aViewId = 7, Nothing())

I am not sure whether the assertion happens only on about:blank or not, if it happens on normal documents, we need the assertions there.

Flags: needinfo?(hikezoe.birchill)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #18)

But with D198684;

  1. UpdateZoomConstraints(aPresShellId = 13, aViewId = 7, a valid ZoomConstraints)
  2. UpdateZoomConstraints(aPresShellId = 14, aViewId = 8, a valid ZoomConstraints)
  3. UpdateZoomConstraints(aPresShellId = 13, aViewId = 7, Nothing())

Huh, that's interesting, I would have thought the calls (2) and (3) come from here and so the Nothing() call comes first.

Anyways, like you said the patch retains the ViewID assertion so if this scenario happens in CI, it should still be caught.

I've already removed the debug build locally, so I can't check it right now, I guess there are two different ZoomConstraintsClient instances.

Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 123 Branch
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f61c77fafa8a
Make title change events block onload in chrome docshells. r=smaug
Blocks: 1875274
Status: REOPENED → RESOLVED
Closed: 1 month ago1 month ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.