Plaintext documents in subframes (or <object>) don't get word-wrapping
Categories
(Core :: CSS Parsing and Computation, enhancement, P3)
Tracking
()
People
(Reporter: karlcow, Assigned: emilio)
References
()
Details
(Whiteboard: [webcompat])
Attachments
(1 file)
So I'm not sure where this one should go, but let's start with the HTML parser. Enter this: data:text/html,<div style="width:200px;"><object data="https://s3-us-west-1.amazonaws.com/fftiers/out/text_RB.txt"></object></div> HTML created for rendering the object element contains: <html> <head> <link rel="alternate stylesheet" type="text/css" href="resource://content-accessible/plaintext.css" title="Wrap Long Lines"> </head> <body> <pre>Tier 1: Todd Gurley, Saquon Barkley, Ezekiel Elliott, Christian McCaffrey, Joe Mixon Tier 2: Nick Chubb, Phillip Lindsay, Alvin Kamara, Leonard Fournette, Dalvin Cook, David Johnson Tier 3: Justin Jackson, Chris Carson, Lamar Miller, Aaron Jones Tier 4: Tarik Cohen, Damien Williams, Derrick Henry, Jaylen Samuels, Sony Michel, Mark Ingram Tier 5: Tevin Coleman, James White, Gus Edwards, Jordan Howard Tier 6: Elijah McGuire, Marlon Mack, Doug Martin, Josh Adams Tier 7: Kenneth Dixon, Jeff Wilson, Matt Breida, Adrian Peterson, Kenyan Drake, Ito Smith Tier 8: Dion Lewis, Jalen Richard, Peyton Barber, Frank Gore, Theo Riddick </pre> </body> </html> We can see the link pointing to resource://content-accessible/plaintext.css But this not interpreted. Initially I thought there was a difference of implementations in between Blink/WebKit and Gecko, but in fact it's a difference just because Firefox seems to not load the CSS.
Reporter | ||
Updated•5 years ago
|
Comment 1•5 years ago
|
||
Are we supposed to load resource://content-accessible/plaintext.css cross-origin? In any case, this isn't the HTML parser.
Assignee | ||
Comment 2•5 years ago
|
||
So this is somewhat weird. That sheet is loaded as an alternate stylesheet, so it shouldn't apply. Looks like for top-level documents we enable it here: https://searchfox.org/mozilla-central/rev/47edbd91c43db6229cf32d1fc4bae9b325b9e2d0/dom/html/nsHTMLDocument.cpp#755 But we intentionally don't do that for subframes (that's the IsChildOfSameType check), see bug 851230. We can change that, I guess... Is there any spec for any of this? Should there be?
Comment 3•5 years ago
|
||
There is no spec. The relevant part of the spec is https://html.spec.whatwg.org/multipage/browsing-the-web.html#read-text and says: User agents may add content to the head element of the Document, e.g., linking to a style sheet, providing script, or giving the document a title. We could certainly switch to always applying plaintext.css, especially if all the other UAs have that behavior anyway.... I'm happy to review a patch that does that and removes the complexity.
Assignee | ||
Comment 4•5 years ago
|
||
I'm happy to write that patch. You can disable wrapping in top-level documents anyway via View -> Page Style -> No style, so it seems like all the title stuff also doesn't make that much sense.
Comment 5•5 years ago
|
||
I think having the title is nice in the sense of it being clear what exactly the sheet does... Though it looks like plaintext.css has grown some bidi stuff that is not line-wrapping related?
Assignee | ||
Comment 6•5 years ago
|
||
This matches other UAs.
Assignee | ||
Comment 7•5 years ago
|
||
Hey Jorg, do you know if thunderbird may rely on plain text documents not wrapping long text by any chance? This patch would change that.
Comment 8•5 years ago
|
||
What do you mean by "plain text documents"? All we render, as far as I know, are HTML documents. You won't change the behaviour of <pre> elements, I assume.
Comment 9•5 years ago
|
||
> What do you mean by "plain text documents"? Documents with type text/plain (or text/css, application/javacript, and a few others like that). > You won't change the behaviour of <pre> elements, I assume. Correct.
Assignee | ||
Comment 10•5 years ago
|
||
I'll land this, happy to provide a way for thunderbird to override this should they need one.
Comment 11•5 years ago
|
||
Backed out changeset 2eab9d9bd89e (Bug 1514655) for geckoview-junit perma failures NavigationDelegateTest.loadData_noMimeType CLOSED TREE Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&selectedJob=218330782&revision=2eab9d9bd89ecf6f26887f01c6df632ac1a255af Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218330782&repo=autoland&lineNumber=3666 Backout push: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=30d8fe076fd0347ef7fb9c1ee7332604ef265f88
Assignee | ||
Comment 12•5 years ago
|
||
Well, that's unexpected. This should have no effect on navigation unit tests... James, seems like you added this test, do you know how to run these, or who could help here? This is somewhat unexpected, but the only explanation I could find is that this GeckoView test was relying on the plaintext.css stylesheet not loading.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #12) > Well, that's unexpected. This should have no effect on navigation unit > tests... > > James, seems like you added this test, do you know how to run these, or who > could help here? This is somewhat unexpected, but the only explanation I > could find is that this GeckoView test was relying on the plaintext.css > stylesheet not loading. The test in question[0] simply loads a gif as a data url with no mime type specified. It's timing out, which suggests that we are not getting either a location change or the page stop notification. Inspecting the logs indicates it's the latter, so I guess something in the patch broke that. You can run these locally with 'mach geckoview-junit`. [0] https://searchfox.org/mozilla-central/rev/9528360768d81b1fc84258b5fb3601b5d4f40076/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt#701
Assignee | ||
Comment 15•5 years ago
|
||
So I tried to do that. Got stuck for quite a while because adb timed out when using su 0. But my rooted phone has su -c. Yet the return code that the python stuff claimed to have was `1`. I ended up giving up and hacking around it with: diff --git a/testing/mozbase/mozdevice/mozdevice/adb.py b/testing/mozbase/mozdevice/mozdevice/adb.py index 01a3c1c516d8..0a8bf0d8fb16 100644 --- a/testing/mozbase/mozdevice/mozdevice/adb.py +++ b/testing/mozbase/mozdevice/mozdevice/adb.py @@ -625,6 +625,7 @@ class ADBDevice(ADBCommand): uid = 'uid=0' # Do we have a 'Superuser' sh like su? try: + self._have_su = True if (self._require_root and self.shell_output("su -c id", timeout=timeout).find(uid) != -1): self._have_su = True But the worst thing is that after so many snags `./mach -v geckoview-junit org.mozilla.geckoview.test.NavigationDelegateTest` passes for me locally with the patch applied of course. I'll try to rebase this patch and get this landed, but in the meantime, should I file a bug for that `su` thing? My setup is fairly simple. I have the latest magick installed (su -v gives 18.0:MAGISKSU (topjohnwu)).
You should try running against an emulator, since that's what automation is using. You can use 'mach android-emulator' to install and run the same emulator that is used in automation. It's failing on 4.3 opt, so you can use 'mach android-emulator --version 4.3' to get the right emulator. It's an ARM emulator, so it will be dramatically slower than a real device (or x86 emulator), so that may be part of the problem.
Comment 17•5 years ago
|
||
Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.
Comment 18•5 years ago
|
||
See bug 1547409. Migrating whiteboard priority tags to program flags.
Comment 19•5 years ago
|
||
Naively, I'd hope that geckoview has changed enough over the past 4 months that the issues here might have gone away? Otherwise, what's a viable path to pushing this over the line?
(I happened to notice the browser.properties load this trips in some logging for an unrelated bug)
Assignee | ||
Comment 20•5 years ago
|
||
Worth a shot, I guess. Will rebase / push to try and check back.
Updated•5 years ago
|
Comment 22•5 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5df5f0db2284 Always wrap plain text documents. r=bzbarsky
Comment 23•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 24•5 years ago
|
||
I assume this ni? is due to the perf regression in that Bing test-case? If so I'm looking into that and already ni?d in the other bug, if not please specify what the ni? was for. Thanks!
Updated•5 years ago
|
Comment 25•5 years ago
|
||
Is the declaration of pref("plain_text.wrap_long_lines", true); still needed in firefox.js and mobile.js after this commit?
Comment 26•5 years ago
|
||
(In reply to Krzysztof from comment #25)
Is the declaration of pref("plain_text.wrap_long_lines", true); still needed in firefox.js and mobile.js after this commit?
I don't think so. Emilio?
Description
•