GNOME Bugzilla – Bug 772831
Clean "reader" view mode
Last modified: 2018-06-22 15:46:10 UTC
Created attachment 337544 [details] screenshot Firefox has this amazing feature built-in: https://support.mozilla.org/en-US/kb/firefox-reader-view-clutter-free-web-pages You click a button and it rips out the main contents of a page so that you have just the contents (the text, hyperlinks, and images), not the crap around it, with a user-controlled visual style. I love the fact that I can set it to a dark theme (white text on dark grey background), with or sans serif, control the text size, column width and line spacing. Epiphany should totally steal their content detection algorithm and have that feature too! And it could maybe steal the theme colors from Gedit/Builder/etc.
So, the actual styling should be quite easy: just steal the CSS from Firefox. GNOME News already has a copy that's been converted to work with WebKit: https://git.gnome.org/browse/gnome-news/tree/data/theme/reader.css and load it using a WebKitUserStyleSheet. The content detection algorithm is another matter, but I'm not certain we even need that as I don't think we need to be pushy about putting a reader icon in the location entry like Firefox does. We could just add a hamburger menu item and a keyboard shortcut and let users decide for themselves which sites it works well on.
Well I dunno, I actually kinda like Firefox's approach of having that button appear contextually if the browser detects it can work... takes the guesswork out of the equation, increases discoverability and reminds to use it, etc. I thought the content detection was needed to apply the stylesheet anyway?
Created attachment 369784 [details] [review] WIP: Clean reader mode implementation Please have a look at this WIP patch.
Review of attachment 369784 [details] [review]: Nice! Honestly, the result seems inferior to Firefox's clean black-on-white reader mode, which uses more reasonably-sized fonts. Try https://git.gnome.org/browse/gnome-news/plain/data/theme/reader.css instead. It might need to be resynced with Firefox's aboutReaderContent.css. ::: embed/ephy-web-view.c @@ +971,3 @@ +{ + WebKitWebView *web_view = data; + EphyWebView *webview = EPHY_WEB_VIEW (web_view); You don't need both variables here. Keep just one, and cast as needed. @@ +974,3 @@ + + webview->js_timeout = 0; + webkit_web_view_run_javascript_from_gresource(web_view, "/org/gnome/epiphany/Readability.js", NULL, NULL, NULL); Missing space before opening parentheses. Also: you should pass a callback, even if all the callback does is print an error message with g_warning() on error. @@ +1298,3 @@ 0); + + g_signal_new ("readable", "readable" sounds like a property name, not a signal name... what should this be named? @@ +1895,3 @@ + + if (!view->in_read_mode) + view->js_timeout = g_timeout_add_seconds (2, run_readability_js, web_view); Why do you need a 2s timeout after load completes? That's weird and annoying. Shouldn't it be possible to run the script immediately? Or use a lower timeout? @@ +2644,3 @@ { + EphyWebView *view = EPHY_WEB_VIEW (web_view); + const char *message = webkit_script_dialog_get_message (dialog); Why is a script dialog being used here? That does not seem right. @@ +2646,3 @@ + const char *message = webkit_script_dialog_get_message (dialog); + + if (strncmp (message, "@EPIPHANY_READER@", 18)) { Where does @EPIPHANY_READER@ come from? How does this work? ::: src/ephy-header-bar.c @@ +638,3 @@ + + if (state) { + gtk_style_context_add_class(style_context, "suggested-action"); I don't think suggested-action really works well here... do we want to suggest the reader should exit reader mode? Try GtkToggleButton instead. Also: missing space before open paren @@ +640,3 @@ + gtk_style_context_add_class(style_context, "suggested-action"); + } else { + gtk_style_context_remove_class(style_context, "suggested-action"); Ditto ::: src/ephy-header-bar.h @@ +42,3 @@ GtkWidget *ephy_header_bar_get_page_menu_button (EphyHeaderBar *header_bar); EphyWindow *ephy_header_bar_get_window (EphyHeaderBar *header_bar); +void ephy_header_bar_set_read_mode_button (EphyHeaderBar *header_bar, EphyWebView *view); This function needs to be renamed... what should its name be? It takes an EphyWebView, not an EphyReadModeButton, so it shouldn't be called _set_read_mode_button(). One argument per line. ::: src/ephy-window.c @@ +2476,3 @@ + g_signal_connect_object (ephy_embed_get_web_view (embed), "readable", + G_CALLBACK (readable_cb), window, G_CONNECT_AFTER); You don't need to cargo-cult G_CONNECT_AFTER here. ::: src/resources/Readability.js @@ +17,3 @@ + +/* + * This code is heavily based on Arc90's readability.js (1.7.1) script Did you make any Epiphany changes to this file? @@ +2114,3 @@ + // BIG HACK but webkitgtk doesn't allow us to read result from js + var previous_title = document.title; + alert("@EPIPHANY_READER@".concat(article.content)); OK, you made some changes. ;) That's a very big hack. Nice that you got it working, of course. You can indeed read the result from JS, so you don't need the script dialog... it will be easier once Carlos's new JSC GObject API lands, but in the meantime you can use the JSC C API. We have a wrapper for this: ephy_embed_utils_get_js_result_as_string().
Thanks for the review Michael, i will update it tomorrow. The important question is how we will handle the read mode. Currently it is using the current webview, which might cause issue with the navigation. An alternative would be to create an own webview for the reader mode and add it as an overlay without touching the navigation...
I didn't test it enough to notice issues with navigation. Try using webkit_web_view_load_alternate_html(). The purpose of that function is to perform an extra load without mucking up the back/forward list.
Created attachment 369811 [details] [review] WIP: Clean reader mode implementation - V2 I've updated the patch based on the current review findings and also added gnome news css style. Please ignore the button within the header bar as it will be replaced by an icon within url entry, it's for testing purpose only. Please give it a try :)
This CSS is much nicer! I do see this nasty error on the terminal: ** (epiphany:10): WARNING **: 18:50:05.171: Error running javascript: ephy-about:overview:2033:25: TypeError: undefined is not an object (evaluating 'this._doc.documentElement')
The location entry should not get unset when switching into reader mode.
Review of attachment 369811 [details] [review]: ::: embed/ephy-web-view.c @@ +91,3 @@ + /* Reader mode */ + char *reader_content; + gboolean in_read_mode; You keep switching between "reader" and "read." I would stick with "reader." in_reader_mode @@ +92,3 @@ + char *reader_content; + gboolean in_read_mode; + guint js_timeout; This variable name is too generic; EphyWebView is a big class. Try reader_js_timeout. @@ +969,3 @@ +static void +readability_finish_cb (GObject *object, + GAsyncResult *result, Align the parameters: (GObject *object, GAsyncResult *result, gpointer user_data) @@ +983,3 @@ + } + + view->reader_content = ephy_embed_utils_get_js_result_as_string (js_result); Yeah, that's a bit nicer than abusing a script dialog for this. :P @@ +985,3 @@ + view->reader_content = ephy_embed_utils_get_js_result_as_string (js_result); + + g_signal_emit_by_name (view, "read-mode-update", NULL); Better, but I don't think you need a signal at all here. Instead, I would add a reader-mode property, backed by priv->in_reader_mode, then you can connect to notify::in-reader-mode instead. @@ +1844,3 @@ + if (!view->in_read_mode) { + g_free (view->reader_content); + view->reader_content = NULL; Use g_clear_pointer(&view->reader_content, g_free) @@ +3644,3 @@ + view->reader_url = g_strdup (ephy_web_view_get_address (view)); + html = g_string_new (""); + GBytes *style_css = g_resources_lookup_data("/org/gnome/epiphany/reader.css", G_RESOURCE_LOOKUP_FLAGS_NONE, NULL); Declare it at the top of the function ::: embed/ephy-web-view.h @@ +151,3 @@ GdkPixbuf *icon); +void ephy_web_view_enable_read_mode (EphyWebView *view, What's the boolean parameter for? If it's called enable_read_mode(), it should just turn on reader mode. @@ +156,3 @@ +gboolean ephy_web_view_is_read_mode_available (EphyWebView *view); + +gboolean ephy_web_view_get_read_mode_state (EphyWebView *view); is_reader_mode_enabled() @@ +158,3 @@ +gboolean ephy_web_view_get_read_mode_state (EphyWebView *view); + +void ephy_web_view_set_read_mode_state (EphyWebView *view, What is the "state"? If it enables/disables reader mode, better call it set_reader_mode_enabled(). But it looks like this function is not defined at all, so the declaration should be removed. @@ +159,3 @@ + +void ephy_web_view_set_read_mode_state (EphyWebView *view, + gboolean state); The indentation is a bit off here. ::: src/resources/reader.css @@ +124,3 @@ +} + +/* FeedView */ I guess this and everything below it should be dropped for Epiphany, and the comment at the top of the file updated.
Please also add a comment to the CSS file on how to get to the upstream source (load chrome://global/skin/aboutReaderControls.css in Firefox). Bonus points if you can display the page title at the top, like Firefox does.
Created attachment 369818 [details] [review] WIP: Clean reader mode implementation - V3 Update including first bonus points. Currently open issue: - Activating reader mode adds it to the history although it should be freezed. Need your help.
(In reply to Michael Catanzaro from comment #11) > Please also add a comment to the CSS file on how to get to the upstream > source (load chrome://global/skin/aboutReaderControls.css in Firefox). > > Bonus points if you can display the page title at the top, like Firefox does. Why is this necessary? The current firefox css has changed alot.
(In reply to Michael Catanzaro from comment #10) > ::: embed/ephy-web-view.h > @@ +158,3 @@ > +gboolean ephy_web_view_get_read_mode_state > (EphyWebView *view); > + > +void ephy_web_view_set_read_mode_state > (EphyWebView *view, State is the "button"/"image" state, as it can/will change during a notebook page switch.
(In reply to Jan-Michael Brummer from comment #12) > Currently open issue: > - Activating reader mode adds it to the history although it should be > freezed. Need your help. When I checked your second patch, it looked like the back/forward list was being maintained properly by webkit_web_view_load_alternate_html(), so I guess the problem is with Epiphany's history service? Did you look at the existing freeze/unfreeze code in EphyWebView? If so, and you're still stuck, I will take a look and try to help. (In reply to Jan-Michael Brummer from comment #13) > Why is this necessary? The current firefox css has changed alot. Good point. I guess it's not needed. (In reply to Jan-Michael Brummer from comment #14) > State is the "button"/"image" state, as it can/will change during a notebook > page switch. I don't understand. Each notebook page has its own web view. The hierarchy is: * EphyNotebook * EphyEmbed * EphyWebView where EphyNotebook has n EphyEmbeds, and each EphyEmbed has one EphyWebView.
(In reply to Michael Catanzaro from comment #15) > (In reply to Jan-Michael Brummer from comment #12) > > Currently open issue: > > - Activating reader mode adds it to the history although it should be > > freezed. Need your help. > > When I checked your second patch, it looked like the back/forward list was > being maintained properly by webkit_web_view_load_alternate_html(), so I > guess the problem is with Epiphany's history service? Did you look at the > existing freeze/unfreeze code in EphyWebView? If so, and you're still stuck, > I will take a look and try to help. Looks like the problem is webkit_web_view_load_alternate_html () with an content_uri set. If it is not set it seems to work. > (In reply to Jan-Michael Brummer from comment #14) > > State is the "button"/"image" state, as it can/will change during a notebook > > page switch. > > I don't understand. Each notebook page has its own web view. The hierarchy > is: > > * EphyNotebook > * EphyEmbed > * EphyWebView > > where EphyNotebook has n EphyEmbeds, and each EphyEmbed has one EphyWebView. Yes, but it used for the button within the header bar, this one is unique and is used for every notebook tab.
I would like to refactor my code and move the handling to the about handler (as firefox does), so that the reader can be accessed by: about:reader?uri=xxxxxx This would perfectly fit with the history handling and re-opens the pages again after epiphany startup. Do you have any objections? (It will also fix my above problem with html loading and titles.)
Sounds like a good idea!
*** Bug 756057 has been marked as a duplicate of this bug. ***
Jan-Michael Brummer , please emulate Microsoft Edge’s read: URI and not Firefox if you want a custom URI protocol. Here is a look at what all the different browsers do (it’s up to date). https://www.ctrl.blog/entry/browser-reading-view-uri Or maybe webread: or epiphany-read:. Also make sure the new protocol is added to the .desktop file and available from external programs. That way, people can open things in reading mode from other applications (like a feed reader or read it later list) by prefixing the reading-mode URI.
The current nightly has broken security indicators after entering reading mode. Click on the padlock in the address field. I suggest either removing the security indicator altogether and replacing it with a reading icon, or mirror the normal webpage’s security indicator.
In the current nightly, double-clicking on the reading mode icon maximizes the window.
Styling of the <mark> element should be less distracting (e.g. blue text instead of yellow background highlighting). E.g. look at this in reading mode, https://www.ctrl.blog/entry/fedora-darkstat
Lastly, I’d also like to ask that you add documentation on how body text, titles, author names, dates, etc. are selected; and how web developers can troubleshoot their compatibility with reading mode. Any web developer who want to make sure their website supports reading mode in Epiphany should have the necessary documentation available to give Epiphany users a good experience. Something like https://wiki.gnome.org/Apps/Web/Docs/Applications but for reading mode. (I don’t have permissions to link bugs so I left my feedback as individual comments here so it wouldn’t get lost. I hope that’s okay.)
(In reply to Daniel Aleksandersen from comment #24) > Lastly, I’d also like to ask that you add documentation on how body text, > titles, author names, dates, etc. are selected; and how web developers can > troubleshoot their compatibility with reading mode. Any web developer who > want to make sure their website supports reading mode in Epiphany should > have the necessary documentation available to give Epiphany users a good > experience. Something like https://wiki.gnome.org/Apps/Web/Docs/Applications > but for reading mode. > > (I don’t have permissions to link bugs so I left my feedback as individual > comments here so it wouldn’t get lost. I hope that’s okay.) Yes, it's OK, but it would be even better to create a bunch of new issues on GitLab for all of the above points. Would you mind reporting new issues at https://gitlab.gnome.org/GNOME/epiphany/issues? That would minimize the chances of your feedback getting lost. I know it's a lot easier to add comments in one issue, but it really helps us to organize tasks. You don't need to create an issue for the custom URI protocol, because the current implementation of reader mode does not use one, but I am reading your blog post now and will keep the feedback in mind. I will close this bug now, since reader mode has landed. Thanks a bunch for testing it! (In reply to Daniel Aleksandersen from comment #21) > The current nightly has broken security indicators after entering reading > mode. I'm particularly surprised that I failed to notice this!
Oh. This was open so I didn’t even think to check in GitLab. Feedback migrated to GitLab. https://gitlab.gnome.org/GNOME/epiphany/issues/38#note_252501 https://gitlab.gnome.org/GNOME/epiphany/issues/44 https://gitlab.gnome.org/GNOME/epiphany/issues/45 https://gitlab.gnome.org/GNOME/epiphany/issues/46 https://gitlab.gnome.org/GNOME/epiphany/issues/47 (In reply to Michael Catanzaro from comment #25) > (In reply to Daniel Aleksandersen from comment #21) > > The current nightly has broken security indicators after entering reading > > mode. > > I'm particularly surprised that I failed to notice this! It’s really not surprising that this would confuse the security indicators, though. Changing origins, extensions, custom protocols? Google Chrome’s new reading mode on Android also got their security indicators all wrong when you enter their reading mode. https://www.ctrl.blog/entry/chrome-simplified-view-android
(In reply to Michael Catanzaro from comment #25) > reader mode has landed. And, for some reason, that made to slashdot today. https://tech.slashdot.org/story/18/06/22/022208/gnome-web-browser-is-adding-a-reader-mode