728563 - Fix inconsistent/confusing feed message open actions

Status: RESOLVED WORKSFORME

(MailNews Core :: Feed Reader, defect)

Description

[alta88]

currently, it's not possible to Open Message in New Window (contextmenu) and have it respect the use set (in subscribe) summary vs. web page default format.  one first has to override the Message -> Open Feed Message, which affects <enter> behavior on a message in the threadpane.

this patch proposes to remove the 2 radio options Web Page and Summary in Message -> Open Feed Message, and make Toggle a checkbox option.

if Toggle is not checked, feed messages are opened as any other according to Options -> Reading and Display [Open messages in], and respect the settings in View -> Feed Message Body as, regarding summary/web page.

if Toggle is checked, it only applies to <enter> or dbl click on the message; contextmenu etc actions are performed like any other message.

(i should have done it this way originally in bug 438429.  there are some bugs that dupe to this.)

blake, does this sound reasonable? perhaps the Toggle checkbox should go in Options rather than Messages menu being different for feeds?

Comment 3

[Ben Bucksch (:BenB)]

Why do you DUP my older, clearly formulated bug against your newer one? Reverting.

Comment 4

[Ludovic Hirlimann [:Usul]]

(In reply to Ben Bucksch (:BenB) from comment #3)
> Why do you DUP my older, clearly formulated bug against your newer one?
> Reverting.

probably because Alta intends on fixing this.

Comment 5

[alta88]

yes, i did intend to fix this.  however, a precondition is that all related bugs are duped to this one, including bug 579533.

Comment 6

[Ben Bucksch (:BenB)]

That's a strange demand, but fine, you can dup it, IF you fix what my bug says.

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Okay, so, I think this sounds reasonable, but I found your description a little hard to follow.  Could you re-phrase it as a set of steps, the expected result, and the actual result?  (That will also help me test your patch.  ;)

Thanks,
Blake.

Comment 10

[alta88]

the whole thing is a bit difficult with many variations.

basically, feeds add 2x the complexity because they can be either summary or web page.

so,
if (<enter>/dbl click && message is feed) {
  if (Toggle Feed In Msgpane is checked && show msgpane)
    do the toggle
  else {
    message to open is [summary || webpage] per global setting || feed setting pref
    open message per Reading/Display->Open Msgs In pref
  }
}

the Toggle Feed In Msgpane checkbox needs to be added in Reading/Display.

there is additional issue with opening web page links, ie if people don't use messagepane and want to see the web page, it is legitimate for it to be opened in
1)content tab
2)default browser
3)new msg window
on just selecting the message.

further, if message pane is used and summary shown by default, then the feed subject/webpage headers should be linkified, to open the web page in
1)content tab
2)default browser
3)new msg window
on both link click and link contextmenu

i thinks those are all the use cases.  an r+ on the design is required before there's a patch.

Comment 11

[Max Polk]

It appears that the only broken thing is the display of the message window.

First choose: View | Feed message body as: | Plain text (and) Summary.

Second choose: Tools | Options | Advanced | Reading & Display | Open messages in: | A new tab.

Success! CORRECT behavior seen hitting <enter> on an article, you see the summary in message tab.

Success! CORRECT behavior seen hitting <F8> on an article, you see the summary in the message pane.

Failure. WRONG behavior seen <right click> on article | Open message in new window, you see a web page with full HTML rendering.

The alternate failure scenario is to choose: Tools | Options | Advanced | Reading & Display | Open messages in: | A new message window.

Failure. WRONG behavior seen hitting <enter> on an article, it loads a web page with full HTML rendering in the new message window.

Remember for news feeds when the user chooses to show no HTML and only the summary, Thunderbird must never load any web page or render any HTML in any window anywhere.

Comment 12

[alta88]

(In reply to Max Polk from comment #11)
> Remember for news feeds when the user chooses to show no HTML and only the
> summary, Thunderbird must never load any web page or render any HTML in any
> window anywhere.

it was not ever designed that render options would apply to external web page content in the same way as to messages.  Adblock Plus is the best solution.

Comment 13

[alta88]

Attachment: patch

this all had to be renovated to fix the print bug. some things:
1)fix the standalone window breakage (from the tabs implementation likely).
2)more/easier options to open web pages in a default browser and for non message pane usage.
3)feed messages in non rss account folders are handled (starting with messages stored in Tb15).

Comment 14

[alta88]

4)feed summary message load is now incremental, not depending on all images/links to load, so display will appear faster.  previously dead links would have prevented message display.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Hey Alta88,

this patch seems to have bitrotted.  Could you get the latest source, and post a new one that applies cleanly?

Thanks,
Blake.

Comment 16

[alta88]

Attachment: rebased patch.

Comment 17

[Jim Porter (:squib)]

I don't know if I'm the best person to look at this, since my knowledge of the RSS implementation is pretty limited. If there's no one else to review this, I can look at it, but I'd be more comfortable reviewing it if there were tests or if the patch were split into smaller logical parts.

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 640319 [details] [diff] [review]
rebased patch.

Myk, you're still listed as a peer for feeds.  Do you still want to be a peer and maybe look at feed stuff?

It seems like there isn't really a lot of risk to the non-feed case here.  The notable change is to FolderDisplayWidget:
  getBrowser().contentDocument.body.hidden = true;

This affects everything (mail too), but seems like a good idea, but obviously requires Blake's sign-off.  I believe displayMessageChanged should be pretty reliable in terms of not wedging us.  Specifically, if it gets called, a message is going to get streamed.  I think our fake tree box should prevent selection changes that try and update the UI, but I am a bit rusty.  Message display suppression is the only complicating factor, and it will trigger message streaming when it has completed.  If things ever do glitch, just changing tabs again or changing the selection should fix them.

It would feel more hygienic to reference the content pane rather than via getBrowser(), but it should be fine.

Comment 19

[Magnus Melin [:mkmelin]]

(In reply to alta88 from comment #14)
> 4)feed summary message load is now incremental, not depending on all
> images/links to load, so display will appear faster.  previously dead links
> would have prevented message display.

Yay!! 

The suite/ parts seem to be missing. I didn't look closely but at least 
http://mxr.mozilla.org/comm-central/source/suite/themes/classic/messenger/messageBody.css#136
http://mxr.mozilla.org/comm-central/source/suite/themes/modern/messenger/messageBody.css#139

Comment 20

[alta88]

Attachment: patch.

5. add support for threadpane context web page opening in browser or content tab/window; apis are already there for such a useful thing.

this patch is not that complex, all it does is change the entry point for the summary/web page display decision to one used by both standalone and 3pane, and makes an object of various formerly global scope polluting vars/funcs. this earlier entry point allows the prevention of double message display 'flash' and removes the former hacky method that broke printing.  the rest is menuitems and strings.

r? to mbanner.

Comment 21

[alta88]

(In reply to Andrew Sutherland (:asuth) from comment #18)

>   getBrowser().contentDocument.body.hidden = true;

for some reason, to me, fixing the mismatched display of message list item different from content makes a big difference.  this should also likely be put in onSelectedMessagesChanged, as switching to a folder with no selection gives the same effect.

> It would feel more hygienic to reference the content pane rather than via
> getBrowser(), but it should be fine.

i'd have to argue quite the opposite ;)


(In reply to Magnus Melin from comment #19)
> (In reply to alta88 from comment #14)
> 
> The suite/ parts seem to be missing.

ah yes.  suite will be a part 2 to this bug.

Comment 22

[alta88]

bit of a collision there.  if mbanner prefers to ask myk for r, he can change it.

Comment 23

[Myk Melez [:myk] [@mykmelez]]

(In reply to Andrew Sutherland (:asuth) from comment #18)
> Myk, you're still listed as a peer for feeds.  Do you still want to be a
> peer and maybe look at feed stuff?

Not particularly, nor am I a particularly good reviewer these days, given that I haven't looked at that code (nor Thunderbird code generally) in so long.

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #23)
> (In reply to Andrew Sutherland (:asuth) from comment #18)
> > Myk, you're still listed as a peer for feeds.  Do you still want to be a
> > peer and maybe look at feed stuff?
> 
> Not particularly, nor am I a particularly good reviewer these days, given
> that I haven't looked at that code (nor Thunderbird code generally) in so
> long.

Okay, I've removed you from https://wiki.mozilla.org/Modules/MailNews_Core#Feeds to save you future hassles.

There is now no module owner or peer, leaving alta88 as the heir apparent if the patches keep coming ;)

Comment 25

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 640701 [details] [diff] [review]
patch.

>+++ b/mail/locales/en-US/chrome/messenger/messenger.dtd
>@@ -451,23 +451,32 @@ you can use these alternative items. Oth
> <!ENTITY recalculateJunkScoreCmd.accesskey "C">
> <!ENTITY openMessageWindowCmd.label "Open Message">
> <!ENTITY openMessageWindowCmd.accesskey "O">
> <!ENTITY openMessageWindowCmd.key "o">
> <!ENTITY openConversationCmd.label "Open in Conversation">
> <!ENTITY openConversationCmd.accesskey "s">
> <!ENTITY openConversationCmd.key "o">
> <!ENTITY openFeedMessage.label "Open Feed Message">
>-<!ENTITY openFeedMessage.accesskey "O">

I'm guessing you removed this because it conflicts with "Open Message"?

>-<!ENTITY openFeedWebPageInWindow.label "Web Page in New Window">
>-<!ENTITY openFeedWebPageInWindow.accesskey "W">
>-<!ENTITY openFeedSummaryInWindow.label "Summary in New Window">
>-<!ENTITY openFeedSummaryInWindow.accesskey "S">
>+<!ENTITY openFeedDefault.label "Open Default Format in New Window or Tab">
>+<!ENTITY openFeedDefault.accesskey "D">
>+<!ENTITY openFeedToggleNonDefault.label "Toggle Non Default Format in New Window or Tab">
>+<!ENTITY openFeedToggleNonDefault.accesskey "N">
> <!ENTITY openFeedWebPageInMP.label "Toggle Web Page and Summary in Message Pane">
> <!ENTITY openFeedWebPageInMP.accesskey "T">
>+<!ENTITY openFeedWebPage.label "Open Feed Web Page in">
>+<!ENTITY openFeedWebPage.accesskey "W">
>+<!ENTITY openFeedWebPageInBrowser.label "Browser">
>+<!ENTITY openFeedWebPageInBrowser.accesskey "B">
>+<!ENTITY openFeedWebPageInContentTab.label "New Content Tab">
>+<!ENTITY openFeedWebPageInContentTab.accesskey "T">
>+<!ENTITY openFeedWebPageInContentWindow.label "New Content Window">
>+<!ENTITY openFeedWebPageInContentWindow.accesskey "W">
>+<!ENTITY openFeedWebPageInBrowserOnSelect.label "Open Feed Web Page in Browser on Message Select">
>+<!ENTITY openFeedWebPageInBrowserOnSelect.accesskey "S">

So, I find the new options more confusing than the previous options.  In general, I don't know what the Default Format is, so using that doesn't help me get what I want.  Also, why can't I just open the non-default format?  Why does it need to be Toggled?  And does Toggling open it?  The "Open Feed Web Page in Browser on Message Select" option seems very invasive, and not particularly useful to me, since it's unlikely people will want to open every feed in their browser, and simple things like selecting a menu option seems to trigger it.  And I can't seem to un-check it now that it's checked!

I think I'ld prefer to keep things a little closer to your original flow:
>if (<enter>/dbl click && message is feed) {
>  if (Toggle Feed In Msgpane is checked && show msgpane)
>    do the toggle
>  else {
>    message to open is [summary || webpage] per global setting || feed setting pref
>    open message per Reading/Display->Open Msgs In pref
>  }
>}

What do you think about changing the names of the menu items, based on the current settings, so that people can see what format they will see, and where it will appear?

Thanks,
Blake.

Comment 26

[alta88]

the major problem here, not honoring web page/summary in a new window, on contextmenu open, is fixed in bug 596234.  anything else can be a new/existing bug.