Closed Bug 1514655 Opened 6 years ago Closed 6 years ago

Plaintext documents in subframes (or <object>) don't get word-wrapping

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Version:
65 Branch
Platform:
Unspecified
Linux
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Project Flags:
Webcompat Priority ?
Tracking Flags:
Tracking Status
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: karlcow, Assigned: emilio)

References

(
URL
)

Details

(Whiteboard: [webcompat])

Attachments

(1 file)

Bug 1514655 - Always wrap plain text documents. r=bzbarsky
47 bytes, text/x-phabricator-request
Details | Review
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.
Summary: Horizontol scrollbar appears in div containing text → when object is called on a text file, the CSS is not applied plaintext.css
Are we supposed to load resource://content-accessible/plaintext.css cross-origin? In any case, this isn't the HTML parser.
Component: HTML: Parser → CSS Parsing and Computation
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?
See Also: → 851230
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.
Summary: when object is called on a text file, the CSS is not applied plaintext.css → Plaintext documents in subframes (or <object>) don't get word-wrapping
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.
Assignee: nobody → emilio
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?
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.
Flags: needinfo?(jorgk)
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.
Flags: needinfo?(jorgk)
> 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.
I'll land this, happy to provide a way for thunderbird to override this should they need one.
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.
Flags: needinfo?(emilio) → needinfo?(snorp)
(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
Flags: needinfo?(snorp)
Alright, I'll give it a shot.
Flags: needinfo?(emilio)
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)).
Flags: needinfo?(emilio) → needinfo?(snorp)
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.
Flags: needinfo?(snorp)

Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.

Webcompat Priority: --- → ?

See bug 1547409. Migrating whiteboard priority tags to program flags.

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)

Worth a shot, I guess. Will rebase / push to try and check back.

Flags: needinfo?(emilio)

Yay, failure isn't there anymore.

Flags: needinfo?(emilio)
Attachment #9032127 - Attachment description: Bug 1514655 - Always wrap plain text documents. → Bug 1514655 - Always wrap plain text documents. r=bzbarsky
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5df5f0db2284 Always wrap plain text documents. r=bzbarsky

https://hg.mozilla.org/mozilla-central/rev/5df5f0db2284

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Regressions: 1548442
Flags: needinfo?(emilio)
Regressions: 1548449

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!

Flags: needinfo?(emilio)

Is the declaration of pref("plain_text.wrap_long_lines", true); still needed in firefox.js and mobile.js after this commit?

(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?

Flags: needinfo?(emilio)

It's not read anywhere, so no.

Flags: needinfo?(emilio)
Blocks: 1565064
Regressions: 1565129
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: