Avoid an URI mismatch when looking for a filter id in the Firefox viewer (bug 1821408) #16144
Conversation
…wer (bug 1821408) We just replace the relative URL by an absolute one.
| #document; | ||
|
|
||
| #id = 0; | ||
|
|
||
| constructor({ ownerDocument = globalThis.document } = {}) { | ||
| this.#document = ownerDocument; | ||
| this.#baseURL = this.#document?.URL ? new URL(this.#document.URL) : null; |
There was a problem hiding this comment.
How "well" will this work for PDF documents opened with a data:-URL, since those can be arbitrary long?
As seen in e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1803050 those URLs can be really problematical, so should we perhaps use document.baseURI instead here?
There was a problem hiding this comment.
baseURI is the problem here: in the builtin viewer, baseURI is resource://pdf.js/web which mismatches with the document URI.
So as far as I can tell we must keep the URL . The comparison is firstly made with scheme and path and in the data case the scheme is data and the path is application/data.... so we really need it.
There was a problem hiding this comment.
So if it becomes a problem (I mean someone complains about that) then we'll see what to do.
There was a problem hiding this comment.
baseURIis the problem here: in the builtin viewer,baseURIisresource://pdf.js/webwhich mismatches with the document URI.
Sorry, but why can't we just update the platform to handle this instead?
Looking at the Firefox code, we already have a bunch of special-cases in both JS and C++ code for that: https://searchfox.org/mozilla-central/search?q=resource%3A%2F%2Fpdf.js%2Fweb&path=
Also, there's even an existing C++ helper to check if something is the built-in PDF Viewer: https://searchfox.org/mozilla-central/rev/2f9a79d1c4ae7090cf50d93be86ba9dc2767a733/dom/base/nsContentUtils.cpp#6987-6995
There was a problem hiding this comment.
It's what I did but the absolute url stuff did the job and I overlooked the data case:
https://phabricator.services.mozilla.com/D172223?id=691180
There was a problem hiding this comment.
Given that this patch won't handle data:-URLs well, I really think that we should fix this on the Firefox side instead.
As mentioned we already have a bunch of "is built-in PDF Viewer"-checks in the Firefox C++ code, using the existing helper, so I don't understand why we can't add one more to address this bug as well; please see https://searchfox.org/mozilla-central/search?q=nsContentUtils%3A%3AIsPDFJS&path=&case=false®exp=false
There was a problem hiding this comment.
An other solution is to remove <base> from:
which is used to compute the url of the filter.
There is a more general discussion on how resolve url in css:
w3c/csswg-drafts#3320
There was a problem hiding this comment.
An other solution is to remove
<base>from:
That seems extremely risky, since any number of things in both the viewer and the platform code most likely depends on that being present; can we please not consider this here.
Still, my question here is what's so "special" about this case that we cannot be allowed to add one more special-case in the platform code?
There was a problem hiding this comment.
An other solution is to remove
<base>from:That seems extremely risky, since any number of things in both the viewer and the platform code most likely depends on that being present; can we please not consider this here.
I agree about the riskyness of removing <base>.
Still, my question here is what's so "special" about this case that we cannot be allowed to add one more special-case in the platform code?
Because ideally it should work without adding such an exception even if we're in a corner case.
@emilio ^^
|
Closing as superseded by PR #16153. |
We just replace the relative URL by an absolute one.