elm / html

Use HTML in Elm!
https://package.elm-lang.org/packages/elm/html/latest/
BSD 3-Clause "New" or "Revised" License
397 stars 99 forks source link

chrome extensions can cause runtime exceptions due to elm.fullscreen #44

Open justgage opened 8 years ago

justgage commented 8 years ago

elm.fullscreen attaches to body. Sadly a lot of chrome extensions actually inject things into the body. This causes errors because the virtual dom goes out so sync with the real dom (that's been messed with). This causes the following error:

(this is an onClick event however anything that changes the dom will mess with it)

error


Main.elm:6446 Uncaught TypeError: Cannot read property 'childNodes' of undefinedaddDomNodesHelp 
@ Main.elm:6446addDomNodesHelp 
@ Main.elm:6454addDomNodesHelp
 @ Main.elm:6454addDomNodesHelp
 @ Main.elm:6454addDomNodes 
@ Main.elm:6400applyPatches 
@ Main.elm:6482updateIfNeeded 
@ Main.elm:5855requestAnimationFrame (async)registerVirtualNode
 @ Main.elm:5834(anonymous function) 
@ Main.elm:3000step 
@ Main.elm:2737work 
@ Main.elm:2793setTimeout (async)enqueue 
@ Main.elm:2782rawSend 
@ Main.elm:2633enqueue
 @ Main.elm:3012eventHandler
 @ Main.elm:6049

I would propose that Elm.fullscreen actually make a "elm-html" div to embed itself into rather using the body itself. I think this would help prevent people from messing with it.

rtfeldman commented 8 years ago

I'm default opposed to accommodating Chrome Extensions (I mean, you mess with the DOM, you're gonna break stuff), but this particular case actually seems like a good idea. Having Elm.Main.fullscreen() be equivalent to Elm.Main.embed(appendEmptyDivToBodyAndReturnIt()) doesn't seem crazy and has separate consistency benefits.

For example, knowing that all Elm programs would be embedded in a div instead of "sometimes div and sometimes body" makes it easier to write a stylesheet that works for both. I've had it happen where I started on fullscreen and then later switched to embed and had to fix my stylesheets. This would prevent that.

evancz commented 8 years ago

In the past, people (I believe including @rtfeldman) expressed frustration with having that additional named div in there, so I took it out. So I'm surprised by this recommendation. I'm not sure what to do given the history of this.

rtfeldman commented 8 years ago

I definitely did! To recap, my specific frustration was having no alternative to writing code like this (specifically the body > div > div > div) for printer stylesheets. There's no way around styling the wrapper div like this if you want the end user to be able to hit Print and have the page look good.

Removing the wrapper was the most direct solution to that problem, since it meant I could just style body and be done with it.

However, @justgage's suggestion (as I took it to mean) of <div class="elm-html"></div> would also provide a reasonable solution to that problem. Having the class would mean that instead of body > div or body > div > div, I could just write .elm-html and style that. Then I wouldn't have to specify my printer-friendly stylesheet in a way that breaks if the body's structure changes (e.g. due to a Chrome Extension).

Although the current approach is simpler, after reading this issue I kinda feel like my advice to newbies ought to be "always use embed and make your own div, don't ever use fullscreen because users' browser extensions will easily break your app."

I feel like a feature that is convenient for beginners but likely to result in bug reports from end users if they deploy to production is worth reconsidering. Better to do something a bit more surprising, a bit less convenient, but also less prone to breaking in common real-world conditions.

rtfeldman commented 8 years ago

Worth noting that I've started always using embed in my teaching materials, because it pre-empts the question I inevitably used to get when I used fullscreen, which was "can I use Elm in part of an existing app, or do I have to take over the whole page?"

If fullscreen genuinely is something best avoided, I'd also support removing it from the API. Anyone who really does need exactly the functionality it provides can use Elm.Main.embed(document.body) anyway.

justgage commented 8 years ago

I've found that I always eventually reach a point at which I need to use embed (want to add a stylesheet, additional javascript, etc...). It would also align better with React's api (although they call it "render" which is sadly not as descriptive of a name). I find myself trying to get by for as long as I can with elm-make which seems to encourage having a fullscreen app, but it does eventually break down.

Although I do agree that it's the chrome extension's fault, I'm not sure if there's anything else it could have done besides modify the body (I don't think chrome allows you to add things like bars and ui over the page otherwise) Also even if the chrome extension is doing something bad end users will blame the site they are visiting, not their extension. Not to mention I was at first blaming Elm by generating a runtime exception which kind of goes against it's "No runtime exceptions" promise. As you can see from the stack trace it's not very obvious where it's coming from.

So the least API breaking change would be to have elm.fullscreen generate a <div>. maybe including some "elm-html" class like you mentioned although that could cause trouble if you have multiple elm embeds on a single page (something I think people trying to transition to elm would want to do.)

The perhaps "better" change would be to deprecate elm.fullscreen in favor of using embed for everything. I think this would help people to know how to render any module in their code besides "Main" and would would pre-empt the questions @rtfeldman mentioned. I would recommend replacing it with a function that would tell people how to upgrade their code rather than just removing it and causing an error (a problem with the upgrade from 0.16 to 0.17.) Perhaps even showing an error in the actual Dom, so they don't have to pop open the console to figure it out.

OvermindDL1 commented 8 years ago

I'd say remove fullscreen, and if anything have an override on embed to take a selector so we could just do Elm.Main.embed("#my_elm_container") would make using it a little more simple.

jtrunick commented 6 years ago

I'm using Browser.document in .19 which I suppose is also prone to this problem? (I am seeing the same error).

hayesgm commented 6 years ago

We're also seeing a high count of users hitting this error on Elm 19. Our set-up is:

Elm.Main.init({
    node: document.getElementById('root'),
});

with HTML:

<html>
  <body>
    <div id="root"></div>
    <script type="text/javascript" src="main.js"></script>
  </body>
</html>

It would make sense that this is due to a browser extension issue, but with such high rates of error, I wonder if there's a work-around.

lisardo commented 5 years ago

I'm facing the same issue as @hayesgm. Any workaround?

carlfredrikhero commented 5 years ago

I also have this problem in 0.19, Browser.Document. I've verified that it is an extension. The incognito mode (which turns off all extensions) does not have the same problem.

polc commented 5 years ago

FYI, I had the same runtime error when using this Chrome extension : https://chrome.google.com/webstore/detail/json-viewer/gbmdgpbipfallnflgajpaliibnhdgobh.

evancz commented 5 years ago

If you are seeing this, please share a link to get the extensions that cause the problem.

That way we can figure out what is causing the root problem and possibly find a solution. I explain a bit of why this would be a good approach here.

berenddeboer commented 5 years ago

I'm seeing this with ChromeVox, an extremely popular screen reader with 163,116 users.

It would be great if we can use Elm to build apps for disabled people too.

And the reason for web developers to use this extension is to check their usage of ARIA controls. The extension says:

Unlike most accessibility software, it is built using only web technologies like HTML5, CSS and Javascript. ChromeVox (Classic) was designed from the start to enable unprecedented access to modern web apps, including those that utilize W3C ARIA (Access to Rich Internet Applications) to provide a rich, desktop-like experience. This enables visually impaired users to experience the power of web applications while also giving developers a way to verify the accessibility of their web applications.

That's the reason I use it.

ChromeVox is also installed by default on every chromebook.

jinjor commented 5 years ago

I'm using Grammarly which automatically checks English grammar. It is also used by 10,000,000+ others.

It inserts an element (like below) when a <textarea> is focused.

image

I found this element is inserted in the middle of body element (not the top or end of body). I guess that this plugin looks for a container element with position: relative to correctly place the icon.

image

Workaround

I found a workaround for this case.

First, I added some helper attributes.

div
    [ style "position" "relative" ] -- let the plugin find this element
    [ span [ class "extension-police" ] [] -- this should be the first child
    , textarea ...
    ]

When .extension-police is not the first element, this function removes the inserted element.

// index.ts

function removeExtensionElement(retryAfter?: number) {
  const polices = document.getElementsByClassName("extension-police") as any;
  let removed = 0;
  for (const p of polices) {
    const firstChild = p.parentElement.firstElementChild;
    if (firstChild !== p) {
      firstChild.remove();
      console.log("removed an extension element");
      removed++;
    }
  }
  if (removed) {
    return;
  }
  if (retryAfter && retryAfter < 1000) {
    setTimeout(function() {
      removeExtensionElement(retryAfter * 2);
    }, retryAfter);
  }
}
document.addEventListener(
  "focus",
  e => {
    // 10, 20, 40, 80, 160, 320, 640
    removeExtensionElement(10);
  },
  true
);
zwilias commented 5 years ago

@jinjor For Grammarly specifically, you can add a data-gramm_editor="false" attribute to your textareas to make Grammarly leave it alone.

noGrammarly : Html.Attribute msg
noGrammarly =
    Attr.attribute "data-gramm_editor" "false"

which can then be used like so:

textarea [ noGrammarly, ...] []

It's fairly annoying to add this to every textarea, but at least it prevents stuff from exploding badly.

jinjor commented 5 years ago

@zwilias Wow, that's really helpful. Thanks!