uBlockOrigin / uBlock-issues

This is the community-maintained issue tracker for uBlock Origin
https://github.com/gorhill/uBlock
915 stars 78 forks source link

Revisiting element picker and about:blank styles on Firefox #143

Closed Fyren closed 6 years ago

Fyren commented 6 years ago

I ran into the same problem others did regarding having a style applied to about:blank in Firefox making the picker/zapper unusable. Only after figuring out the cause myself was I able to actually find https://github.com/uBlock-LLC/uBlock/issues/806 (adding "about:blank" to my searches made all the difference).

gorhill commented the following in the linked issue:

So anyways, the issue was with about:blank customization, something out of control of uBlock. Right?

Customization of about:blank is out of uBlock's control, but using about:blank is not. As far as I can tell, the srcless iframe the picker creates is the reason about:blank gets used, but I don't think that's necessary. I don't know what the best method is, but adding rootPicker,srcdoc = ""; after creating the iframe (https://github.com/gorhill/uBlock/blob/master/src/js/scriptlets/element-picker.js#L1608) seemed to have no impact except ending up with my about:blank style not applied. I also tried a data URI for src instead, but that appeared to break the picker, but perhaps I crafted the data URI incorrectly. I only tested on Firefox nightly.

Can something like this be added if there's no regression caused by it? I think having about:blank styles applied to the picker is extremely unintuitive and difficult to diagnose, so this would prevent at least a small number of users some pain.

uBlock-user commented 6 years ago

Duplicate of https://github.com/gorhill/uBlock/issues/826

Fyren commented 6 years ago

It's not-a-PR since I didn't want to create one out of the blue (I have no idea if there's guidelines for that, didn't see any). Maybe that makes it a suggestion.

Fyren commented 6 years ago

The CSS rule suggested in the issue this is marked as a dupe of requires the user knows what the problem is in the first place. The one-liner code change I gave does not, but I don't know if it would inadvertantly break anything.

uBlock-user commented 6 years ago

I don't know which version of Firefox you're on, but I'm on Nightly and the Block Element option simply doesn't appear on about:blank in the context menu neither when right clicked on the uBO icon, so you simply can't invoke Picker in the first place which I think is the desirable behaviour rather than having picker being messed up with user-styles set by the user and then adding CSS hacks to make it usable. Same for Zapper.

Fyren commented 6 years ago

I'm on nightly, as mentioned in my original comment. If you left click on the addon button you get something like this: https://github.com/gorhill/uBlock/blob/master/doc/img/df-qg-01.png

It's a little different in a modern version, but the eyedrop/lightning bolt icons are for the picker and zapper.

uBlock-user commented 6 years ago

If you left click on the addon button you get something like this

Not on about:blank, you don't. Also that's the outdated version of uBO, no longer valid.

This is it how it is now -

Only Dashboard and Logger icons are available.

Fyren commented 6 years ago

The problem is not about using the picker on about:blank, it's that if you use the picker anywhere the style you have set on about:blank applies to the picker.

Fyren commented 6 years ago

To try to clarify, that's why I don't think someone will realize they need to use the given CSS rule. They probably won't realize the about:blank style has anything to do with uBlock's picker at all. The suggested change makes it so uBlock's picker doesn't end up being considered about:blank.

uBlock-user commented 6 years ago

Whether to add that suggested change of yours is upto gorhill.

gwarser commented 6 years ago

Seems to work, but custom style is flashing for a moment.

gorhill commented 6 years ago

Please provide the exact user CSS you are using, and in which file, so that I can reproduce your exact issue on my side. If I remember correctly uBO uses user styles to enforce the picker styles, so I need a repro case to investigate. The duplicate issue above was probably when user styles were not used by uBO.

Fyren commented 6 years ago

I had @-moz-document url-prefix(about:blank) { * { background-color: black !important; } } in userChrome.css, resulting in the entire page looking black besides the red/yellow element overlays.

gorhill commented 6 years ago

I can't reproduce, I added the above CSS snippet to chrome/userChrome.css in the root of the profile and the picker is still transparent.

Fyren commented 6 years ago

Sorry, chrome/userContent.css. userChrome is for the browser UI itself, userContent for rendered content. I just verified on a new profile. I'm unsure if restarting Firefox is required after changing the file.

gorhill commented 6 years ago

According to the inspector, uBO is overriding the style in userContent, but somehow this is not really the case because the iframe is no longer transparent:

a

Fyren commented 6 years ago

I see the same for the HTML and body tags, but the svg that covers the entire viewable area is getting the background-color from the userContent rule.

gorhill commented 6 years ago

My view on this is that a style such as * { background-color: black !important; } for about:blank is going to cause issues to more than just uBO, it's too indiscriminatory. about:blank frames are anonymous frames, which inherit the parent's policies, and they are legitimately used for all sort of purposes by not only extensions such as uBO, but also in normal web pages. For example, an about:blank frame on https://news.ycombinator.com/ is effectively a frame from https://news.ycombinator.com/.

If your real intention is to have empty about:blank pages painted a specific color, than you need to account for the fact that about:blank might not be an empty document, like so:

@-moz-document url-prefix(about:blank) {
    html > body:empty {
        background-color: black !important;
        margin: 0 !important;
    }
}

The issue with the former style is not specific to uBO, the issue is the style used itself.

Fyren commented 6 years ago

While the HTML5 spec appears to say that if there's no srcdoc and no src, then the src is considered to be about:blank, but I don't think the intent is about:blank to be a canvas for developers to use. The only spec I can see that might apply is RFC6694 which says the following:

The "about:blank" URI refers to a resource represented in the browser by a blank page.

If I style about:blank, I'm styling such a resource. If you start using about:blank to display content, then that's misusing because you just want to start from a blank page rather than display about:blank.

gorhill commented 6 years ago

I don't think the intent is about:blank to be a canvas for developers to use

It's actually used a lot like this. Dynamically iframes are created as about:blank, i.e. empty document, but nothing whatsoever prevent the created iframe to have its DOM dynamically populated, and it's actually commonly done.

I consider your issue to be your indiscriminate styling, it needs to be more focused as shown, it's not a uBO-specific issue, if you think so you will end up reporting all such cases to countless places, including on web sites.

Fyren commented 6 years ago

I think I've had that rule there for years without running into a problem till now with uBO. In a pragmatic sense, there have been several users who ended up making issues about this, so there have certainly been more that haven't, and it could be prevented by intentionally avoiding using about:blank by setting an empty srcdoc (or possibly other ways).

gorhill commented 6 years ago

I've had that rule there for years without running into a problem till now with uBO

But this does not really matter. What matter is that your custom CSS rule is telling the browser to override the background color of all about:blank iframes, regardless of whether they contains an actual DOM with a precise purpose. Your custom rule assumes wrongly that all about:blank frames are empty DOM, which is a false assumption -- there is no spec out there which says the DOM of about:blank iframes must never be populated.

It does not matter that you never stumbled onto such a case, the assumption is still wrong, and your custom rule resulting from the wrong assumption needs fixing.

The purpose of about:srcdoc is to provide the HTML content as a value of the srcdoc attribute -- you are just asking me to modify uBO to make it work with your too-broad CSS rule. It's just a matter of time before someone else show up here and ask me to fix uBO for their custom CSS about:srcdoc rule -- which would also be wrong.

The key issue is, do not override the CSS styles of non-empty documents, it's for you to fix, your rule is too indiscriminate, and as expected it created an issue.

Bottomline is that it's cool that Firefox allows such customization, but whoever uses these cool features will have to bear the burden of the consequences. It makes no sense to ask all those who uses anonymous iframes out there to mind all those potential customization which are out of their scope.

Fyren commented 6 years ago

The difference is that the clearly-given purpose of about:srcdoc is that it's the stand-in URL for documents generated from the attribute. I agree that broadly styling it is dubious, because the user would be saying "I want to style whatever came through a srcdoc element." When I have a style for about:blank, even if broad, that's saying "I want to style whatever's on this page named about:blank."

By not setting either src or srcdoc, you're asking to display about:blank which isn't really what is desired; you want to display the overlay for the picker. You've specifically chosen "apply anything that does URL-based matching against about:blank" rather than "this page doesn't count as about:blank." Bringing up others picking about:blank as an anonymous or dynamic source is basically an "if everyone else is jumping off cliffs, then yeah, I'd do it, too" argument. Basing it the overlay on docsrc, or even some javascript: or data: or other URI all seem to make more sense than about:blank (and are all closer being anonymous).

By this point, I assume we're not going to convince each other, but thank you for discussing it.

gorhill commented 6 years ago

Quick example, your custom CSS rules breaks the rendering of embedded tweets on this page: https://www.watson.ch/Digital/Twitter/531665406-Das-sind-die-10-beliebtesten-Tweets-des-Jahres-%E2%80%93-Obama-ist-gleich-3-Mal-dabei#twitter-widget-2.

However trivial is the change, I won't modify uBO's code for the principle: your overly broad CSS rule is wrong in the first place, it makes the assumption that about:blank is always an empty DOM (but I am just repeating myself), "fixing" uBO to accommodate your specific CSS rule which unduly interfere with extensions/pages would open the door to start accommodating all such issues external to uBO.