1341456 - Toggling "Sans-serif"/"Serif" works only after scrolling the page

Status: VERIFIED FIXED

(Firefox for iOS :: Reader View, defect, P1)

Description

[Adrian Oniga]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Steps to reproduce:

1.Open Firefox 
2.Go to cnn.com
3.Open an article in "Reader mode"
4.Tap on the "Aa" icon
5.Toggle "Sans serif"/"Serif" 



Actual results:

2.Cnn.com is opened
3.Article in "Reader mode" is opened 
4.Contextual menu appears
5.Nothing happens. Toggling "Sans-serif"/"Serif" works just after scrolling the page


Expected results:

5."Sans-serif"/"Serif" is toggled and text change is visible imediatly, without having to  scroll the page

Comment 1

[Adrian Oniga]

Found on iPhone 6, iOS 10.2.1, Firefox version 7.0(1), buddybuild #1696

Comment 2

[Simon Basca [:SimonB]]

I can confirm that this issue also occurs on latest master a15ada2.

Comment 3

[Aaron Train [:aaronmt]]

Swift 3 migration regression?

Comment 4

[:sdaswani]

I can't find any error case or different behavior that we can switch on in the case where the style change doesn't work (page hasn't been scrolled yet) vs. the case where the style change does work (page has been scrolled). Neither the swift processing (https://github.com/mozilla-mobile/firefox-ios/blob/bf88f9faf693ee2d0e9a20244c8fc3253da31ebe/Client/Frontend/Browser/BrowserViewController.swift#L2684) nor the javascript processing (https://github.com/mozilla-mobile/firefox-ios/blob/dae882fbadd4e467f7a6e4f04007528931fffddc/Client/Frontend/Reader/ReaderMode.swift#L291) act any differently in the two cases, AFAICT.

I have a sneaking suspicion that WKWebView disallows DOM changes via javascript until the page has been scrolled / interacted with, but I need to do research to confirm this.

I will punt on this until I can discuss it further with Steph or anyone else who has any insight on the issue (feel free to chip in ;) ).

Comment 5

[Stephan Leroux [:sleroux]]

> I have a sneaking suspicion that WKWebView disallows DOM changes via javascript until the page has been scrolled / interacted with, but I need to do research to confirm this.

I have the same feeling looking this over. It seems that our JS code for toggling the style indeed executes correctly but the DOM just doesn't realize until a scroll event occurs so I doubt this is related to the Swift 3 migration.

I'd be interested to see if this has always been an issue since I don't see what could have changed to cause this to happen now.

Comment 6

[:sdaswani]

(In reply to Stephan Leroux [:sleroux] (PTO Mar 13-17) from comment #5)
> > I have a sneaking suspicion that WKWebView disallows DOM changes via javascript until the page has been scrolled / interacted with, but I need to do research to confirm this.
> 
> I have the same feeling looking this over. It seems that our JS code for
> toggling the style indeed executes correctly but the DOM just doesn't
> realize until a scroll event occurs so I doubt this is related to the Swift
> 3 migration.
> 
> I'd be interested to see if this has always been an issue since I don't see
> what could have changed to cause this to happen now.

I actually am not seeing this behavior in the App Store version (6.1).

Comment 7

[:sdaswani]

I'm attempting to move this to the backlog but not sure I've demarcated it correctly.

Comment 8

[Jacob White]

I was looking at this the other day in the simulator. It's not happening in the App Store version on device or on 7.0(2125) on device.

Comment 9

[:sdaswani]

I was definitely able to reproduce this in both the simulator and on my device last week when building through XCode, but I'll try with the 2125 build and update this thread.

Comment 10

[:sdaswani]

Wow, I can't reproduce this with 2125 on my iPad, but I can reproduce it on my 7Plus. So the plot thickens (and gets more confusing)....

Comment 11

[Justin D'Arcangelo [:justindarc]]

Attachment: GitHub Pull Request

Very, very weird bug. For some reason, WebKit seems to want the `font-face` name enclosed in quotes.

Comment 12

[Justin D'Arcangelo [:justindarc]]

Hmmm.. Wait a minute.. It doesn't look like our custom serif/sans-serif fonts are being loaded at all?

Comment 13

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8849711 [details] [review]
GitHub Pull Request

A-ha! That's the problem. Looks like a content security policy issue:

Failed to load resource: the server responded with a status of 405 (Method Not Allowed)

Unflagging this patch for review for now until I figure out what's going on with this.

Comment 14

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8849711 [details] [review]
GitHub Pull Request

Ok, fixed for real this time. "Optional()" strikes back!! :-/

Comment 15

[Stephan Leroux [:sleroux]]

Comment on attachment 8849711 [details] [review]
GitHub Pull Request

LGTM

> "Optional()" strikes back!! :-/

Can't wait for Optional(): Revenge of the Swift!

Comment 16

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I added a comment to the patch https://github.com/mozilla-mobile/firefox-ios/pull/2546/files#r107818955

Comment 17

[Stephan Leroux [:sleroux]]

master e59e447e4b58523367e43e4749990e2ffe33b935
v7.x f9abb15

Comment 18

[Adrian Oniga]

Verified as fixed on iPhone 6, iOS 10.2.1, Firefox 8.0(1), buddybuild #2356