1935609 - c.spotler.com - Clicking the highlighted button does not work

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[Calin Tanase]

Environment:
Operating system: Windows 10
Firefox version: Firefox 134.0/133/135

Steps to reproduce:

1. Go to https://c.spotler.com/ct/m9/k1/bQwqrDDSYmkgqatJmeWL-J5ZnedEvwmcKJ3SMb3ITn6CwLhxmZjVQG4BXQCviO_Z/Shu4AAW4Jd9mDVt
2. Click on the highlighted button (orange).

Expected Behavior:
Link opens in a new tab.

Actual Behavior:
Nothing happens.

Notes:

• Reproduces regardless of the status of ETP
• Reproduces in firefox-nightly, and firefox-release
• Does not reproduce in chrome

Created from https://github.com/webcompat/web-bugs/issues/144834

Comment 1

[Calin Tanase]

Attachment: FF vs Chrome.mp4

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

Comment 3

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=962f5bdec7f48c7bcf4c317934e299f5a97d9b4d&tochange=3c40aaebf025a776c32b497507d579b0bc86c127

Suspect: Bug 1595491

<a style=" text-decoration: none; text-decoration-color: rgb(0, 0, 238); text-decoration-style: solid;" rel="noopener noreferrer" href="https://c.spotler.com/ct/m9/k1/mFc6haoxFuPcZTFrFMO2jK2v8cMwgjmC7xvLqaazF0Y/7E3cIT4ZpSjf4tL" target="_blank">
  <object data="https://content.mailplus.nl/m9/images/ts/scoutshop/2024___Decemberkalender/images/cd-06-12-2024-F.jpg" width="140" height="140"></object>
</a>

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

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

:sefeng, since you are the author of the regressor, bug 1595491, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 5

[Timothy Nikkel (:tnikkel)]

If you disable fission does this work?

Comment 6

[Alice0775 White]

(In reply to Timothy Nikkel (:tnikkel) from comment #5)

If you disable fission does this work?

yes, set fission.autostart to false, it works as expected.

Comment 7

[Dennis Schubert [:denschub]]

The <a> doesn't have a height, but this seems unrelated. If I manually specify dimensions, I still can't click. But if I remove the <object>, I can click.

Comment 8

[James Graham [:jgraham]]

FWIW if you replace <object data= with <img src= the entire site works. They seem to be using an <object> to load an image to change the background color of a table cell.

Comment 9

[Sean Feng [:sefeng]]

I am pretty sure Andreas knows more about these, so delegating to him :)

(Thanks Andreas!)

Comment 10

[Andreas Farre [:farre]]

I guess that the problem is that it's not just an <object> loading an image, it's an <object> loading a cross origin image :/. I'll start with throwing together a wpt test for this.

Comment 11

[Andreas Farre [:farre]]

The same issue happens with:

<a href="https://example.com">
  <iframe src="http://elg.no/elg.jpg"></iframe>
</a>

and the reason seems to be that the document ends up in a layer on top of the anchor. And this is basically how <object>/<embed> works with cross origin images. Emilio, this is a bit out of my league, do you have a quick solution or pointers?

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Dennis Schubert [:denschub] from comment #7)

The <a> doesn't have a height, but this seems unrelated. If I manually specify dimensions, I still can't click.

Can you elaborate? Clicking works fine here if I set height: 140px; width: 140px; display: inline-block. You need to set display because otherwise width/height don't applies to bare inlines.

(In reply to Andreas Farre [:farre] from comment #11)

and the reason seems to be that the document ends up in a layer on top of the anchor. And this is basically how <object>/<embed> works with cross origin images. Emilio, this is a bit out of my league, do you have a quick solution or pointers?

Well, so this seems basically bug 1909584, but with an extra caveat which is that somehow we are not hitting the remote iframe background when remote, right?

So first thing I'd try is nerfing this condition here. I'm not sure it's needed anymore but Tim or someone on the webrender side would probably know. If removing that fixes it, then great.

Otherwise, I'd look into the apz code, maybe this is not doing the right thing in this case or something.

Comment 13

[Andreas Farre [:farre]]

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

Otherwise, I'd look into the apz code, maybe this is not doing the right thing in this case or something.

Yeah, this is my bet. I'm not really sure how to proceed here, though. Having <object>/<embed> behave like <iframe>s were bound to trigger unknown behaviour. It only happens for cross origin images. That is, with same origin images it works fine. Interestingly enough I discovered that Chrome doesn't handle this, and neither the intrinsic size.

Emilio, do you have a candidate for someone to take this, I'm unfortunately clueless when it comes to apz. Or maybe you know Hsin-Yi?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Test-case

So looking a bit deeper into it I think it's actually not an APZ bug. In this test-case nsSubDocumentFrame::ContentReactsToPointerEvents returns true, because IsSyntheticImageDocument() returns false, because mContentType is text/html here.

Andreas, tweaking that check fixes it for me. Do you know what the right thing to do there is? Checking mOriginalContentType? Or add some more extra state around here?

Comment 15

[Andreas Farre [:farre]]

Attachment: clickable-anchor-object.mp4

I forgot to mention that I'd actually tried this, but my solution was:

--- a/layout/generic/nsSubDocumentFrame.cpp
+++ b/layout/generic/nsSubDocumentFrame.cpp
@@ -1269,12 +1269,10 @@ bool nsSubDocumentFrame::ContentReactsToPointerEvents() const {
   if (Style()->PointerEvents() == StylePointerEvents::None) {
     return false;
   }
-  if (mIsInObjectOrEmbed) {
-    if (nsCOMPtr<nsIObjectLoadingContent> iolc = do_QueryInterface(mContent)) {
-      const auto* olc = static_cast<nsObjectLoadingContent*>(iolc.get());
-      if (olc->IsSyntheticImageDocument()) {
-        return false;
-      }
+  if (mIsInObjectOrEmbed && mFrameLoader) {
+    if (BrowsingContext* browsingContext = mFrameLoader->GetBrowsingContext();
+        browsingContext && browsingContext->GetSyntheticDocumentContainer()) {
+      return false;
     }
   }
   return true;

which essentially is the same thing. I actually agree with you though that this test should be in nsObjectLoadingContent::IsSyntheticImageDocument. I'll attach a WIP patch here for that. But the thing is, it only half-works (or actually three quarters work). The image in the <objectg> element becomes clickable (as in that the <a> gets clickable). But in the attached video you'll see that the pointer only changes if you enter the image from below.

Comment 16

[Andreas Farre [:farre]]

Attachment: Bug 1935609 - Part 2: Use browsing context's synthetic document status. r=emilio!

Comment 17

[Emilio Cobos Álvarez (:emilio)]

(In reply to Andreas Farre [:farre] from comment #15)

The image in the <object> element becomes clickable (as in that the <a> gets clickable). But in the attached video you'll see that the pointer only changes if you enter the image from below.

Yeah I can see that too. I think that's probably a separate bug... Can you file a follow-up for that? I think it's also a lot less severe

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Should this patch be landing under another bug? IIUC, Firefox patches shouldn't be landing under the Web Compat product.

Comment 19

[Andreas Farre [:farre]]

Attachment: Bug 1935609 - Part 1: Rename synced field to IsSyntheticDocumentContainer. r=emilio!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73c9808d9d62
Part 1: Rename synced field to IsSyntheticDocumentContainer. r=emilio
https://hg.mozilla.org/integration/autoland/rev/7badd7f689d2
Part 2: Use browsing context's synthetic document status. r=emilio
https://hg.mozilla.org/integration/autoland/rev/39d33e72d8b4
apply code formatting via Lando

Comment 21

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/73c9808d9d62
https://hg.mozilla.org/mozilla-central/rev/7badd7f689d2
https://hg.mozilla.org/mozilla-central/rev/39d33e72d8b4

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Does this need Beta approval requests? Please nominate if yes.

Also moving this bug into to the correct component since a platform fix landed in it rather than a dependent bug. NI Honza to make sure this gets the appropriate webcompat flags set on it.

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:farre, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox135 to wontfix.

For more information, please visit BugBot documentation.

Comment 24

[Andreas Farre [:farre]]

Comment on attachment 9459860 [details]
Bug 1935609 - Part 1: Rename synced field to IsSyntheticDocumentContainer. r=emilio!

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Images embedded in object or embed elements can't be used in anchor elements.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small and only related to object/embed elements.
• String changes made/needed:
• Is Android affected?: Yes

Comment 25

[Andreas Farre [:farre]]

Comment on attachment 9459545 [details]
Bug 1935609 - Part 2: Use browsing context's synthetic document status. r=emilio!

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Images embedded in object or embed elements can't be used in anchor elements.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small and only related to object/embed elements.
• String changes made/needed:
• Is Android affected?: Yes

Comment 26

[Andreas Farre [:farre]]

Both parts are needed, since 2 depends on 1.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9459860 [details]
Bug 1935609 - Part 1: Rename synced field to IsSyntheticDocumentContainer. r=emilio!

Approved for 135.0b8.

Comment 28

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/ce2316f2be92
https://hg.mozilla.org/releases/mozilla-beta/rev/dd195601d610