1200602 - Don't show the missing-plugin UI for <applet> with an unknown MIME type

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Masatoshi Kimura [:emk]]

Steps to reproduce:
1. Install Win64 Firefox.
2. Open any page that contains a Java applet.

Actual result:
A useless missing plug-in box.

Expected result:
The fallback content should be displayed for web authors to write something more useful.
Microsoft Edge works as expected.

Comment 1

[Benjamin Smedberg]

Agreed! I put a spec in the user-story block. I don't think I have paid engineers to work on this right now, but I bet I can find a mentor at least!

Comment 2

[Masatoshi Kimura [:emk]]

This bug will effectively revert bug 425013.

Comment 3

[Masatoshi Kimura [:emk]]

For an <embed>, fallback content will never be present because <embed> has no ability to have fallback content.
We should also change the <applet> behavior.

Comment 4

[Masatoshi Kimura [:emk]]

Hm, looks like an <object> already displays the fallback content correctly. An <embed> cannot have any fallback content. The HTML spec requires to display something visible to user for an unsupported <embed>.
https://html.spec.whatwg.org/multipage/embedded-content.html#the-embed-element:the-embed-element-14
> The embed element has no fallback content. If the user agent can't find a suitable plugin when
> attempting to find and instantiate one for the algorithm above, then the user agent must use a
> default plugin. This default could be as simple as saying "Unsupported Format".
So only an <applet> does matter.

Comment 5

[Chris Peterson [:cpeterson]]

Masatoshi, do you have a link to an test page that has <applet>, <embed>, and/or <object> with unknown MIME types?

Comment 6

[Benjamin Smedberg]

hrm, if there is no fallback content I wonder if we can go to the broken-image icon instead of completely invisible.

I don't care what the HTML spec says, we aren't going to show users any text in this situation.

Comment 7

[Benjamin Smedberg]

Kyle can you point potential contributors at the place in the code where this would be implemented?

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: Testcase

* Edge displays nothing if fallback text is unavailable.
* Internet Explorer 11 works as expected.
* Chrome 44 did not display a missing-plugin box nor fallback text and displayed an infobar.

Comment 9

[Masatoshi Kimura [:emk]]

Please test with Java uninstalled because <applet> cannot customize the MIME type.

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, so I haven't dealt much this code, but here's my best guess to the future mentee of this bug:

We're interested in the DOM element that loads the plugin or throws an error if it can't be loaded for some reason, which is DOMObjectLoadingContent. The logic for figuring out what we display on various plugin failure cases is in dom/case/DOMObjectLoadingContent.cpp. To display the alternative text we're looking for, we'd usually use the eFallbackAlternate error code. However, the bug here is that we're using some other error code. So, I /think/ fixing this bug means finding the logic related to applets and changing it to use the right error code.

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: Use the alternate content for <applet>

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8657950 [details] [diff] [review]
Use the alternate content for <applet>

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

Looks good, and seems to work for me! Just have one small comment, and honestly the patch could be checked in this way if the style is ok.

However, before we do a checkin-needed on this, it'd be nice to have a test too, to make sure that we're rendering the right thing. If you look at dom/base/test/test_810494.html, I believe you should be able to adapt that to test for the issues in this bug. Just submit another patch with that test with me as reviewer. If you need more info on mochitests, feel free to fb? me here or I'm qDot on irc, though the MDN mochitest page is pretty through too.

::: dom/base/nsObjectLoadingContent.cpp
@@ +2961,5 @@
>      // determined type should always just be alternate content
>      aType = eFallbackAlternate;
>    }
>  
> +  if (!thisContent->IsHTMLElement(nsGkAtoms::embed) &&

Nit: Ok, so I'd probably err on the side of verbosity here and have

(thisContent->IsHTMLElement(nsGkAtoms::object) ||
thisContent->IsHTMLElement(nsGkAtoms::applet)) &&

as it just makes it more obvious that we want the two tags we know have fallbacks. I realize we probably would never hit this code in the first place in anything that's not an object/applet/embed, though, so the way it currently is would work too. Mostly a style preference issue. :)

Comment 13

[Masatoshi Kimura [:emk]]

Attachment: Use the alternate content for <applet>, v2

- Test added.
- Changed !isembed to isobject||isapplet.

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8659279 [details] [diff] [review]
Use the alternate content for <applet>, v2

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

Looks good! I'll get a try run going, once that's done we can land it.

Comment 15

[Masatoshi Kimura [:emk]]

Try looks green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=72a4a2dcca98

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa5459a6703d

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aa5459a6703d

Comment 18

[Masatoshi Kimura [:emk]]

Comment on attachment 8659279 [details] [diff] [review]
Use the alternate content for <applet>, v2

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Some web pages will confuse Win64 Fx users if they use a Java applet. A missig plugin box will be displayed, but users have no way to enable Java on Win64 Fx.
[Describe test coverage new/current, TreeHerder]: Tested locally and m-c
[Risks and why]: Low. The new code path is already used for the <object> element.
[String/UUID change made/needed]: none

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8659279 [details] [diff] [review]
Use the alternate content for <applet>, v2

Improve the quality of the Windows 64 port, taking it.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/90aa85f77690

Comment 21

[Masatoshi Kimura [:emk]]

I filed bug 1204854 for comment #6.