550794 - View > Feed Message Body As: incorrect/unexpected re-arrangement of grouped pref menus in 'Summary' mode (dominating feed layout pref should always remain at the top, per ux-visual-hierarchy)

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Mike Cowperthwaite]

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.2.2pre)
 Gecko/20100307 Lanikai/3.1b2pre

When an RSS item is selected, the menu
  View | Message Body As >>
changes to
  View | Feed Message Body As >>

This menu offers  Summary, Web Page, Default Format  as the options.

If |Summary| is selected, or if |Default Format| is selected for a feed configured to show Summary, the menu *also* shows the older options:
 x Original HTML
   Simple HTML
   Plain Text
   ----
   Web Page
 x Summary
   Default Format

I don't know if this is intended; I doubt it's even necessary, as my feeds' summaries already look like they're in highly vanilla HTML.  No matter what selection I make of those older options, the display of the item summary does not change.  Changing this selection in an RSS folder does not change the selection in a mail folder, but it does persist when changing to another RSS folder (so long as the Summary mode is in use).

OTOH, it would be nice to be able to apply "simple HTML" to Web Page formatting -- not sure if this is feasible in the current architecture or not.

[when trying to reproduce this bug, see bug 550793 which can confuse the results here]

This behavior also presents in 3.03.  Running 32-bit TB on 64-bit WinXP here.

Comment 1

[Ben Bucksch (:BenB)]

> my feeds' summaries already look like they're in highly vanilla HTML

Not sure I understood correctly, but "Simple HTML" is mainly a security feature and only secondarily a 'cleaner rendering' feature, so it's irrelevant what HTML *usually* appears in RSS feeds. The option makes sense for any HTML.

Comment 2

[Mike Cowperthwaite]

(In reply to comment #0)
> I don't know if this is intended

I have now read bug 438429 comment 20 and it is indeed intended: Original/Simple/Plain are supposed to be options only for the Summary display.

I still don't think this "Feed Message Body" menu is right.  Currently, it shows as
   Web Page
   Summary
   Default Format

if "Web Page" is selected or if "Default Format" is selected and the feed folder's default is "Web Page"; the Original/Simple/Plain pref is not shown at all.  But for the "Summary" case, the Original/Simple/Plain pref is shown, and shown on top which doesn't make sense.

My suggestions for this menu are:

1) Put the Web Page/Summary/Default selection first within the menu, since it's the controlling pref
2) Rather than hiding the Original/Simple/Plain selection, grey them out when the first preference means they don't apply.

And also fix bug 550793.

Comment 3

[Thomas D. (:thomas8)]

(In reply to Mike Cowperthwaite from comment #2)
> I still don't think this "Feed Message Body" menu is right.  Currently, it
> shows as
>    Web Page
>    Summary
>    Default Format
> 
> if "Web Page" is selected or if "Default Format" is selected and the feed
> folder's default is "Web Page"; the Original/Simple/Plain pref is not shown
> at all.

Mike, as you reported, for "Web page", the Original/Simple/Plain menus are not shown, which is somewhat against expectation. Is it intended *not* to show the flavors, or they should be shown? Shouldn't we show/enable the flavors whenever there's HTML in the body?

> But for the "Summary" case, the Original/Simple/Plain pref is
> shown, and shown on top which doesn't make sense.

Also, it doesn't work. In "Summary" mode, toggling Original/Simple/Plain does not change anything for me - always seeing the same HTML body/footer.

> My suggestions for this menu are:
> 
> 1) Put the Web Page/Summary/Default selection first within the menu, since
> it's the controlling pref

+1

> 2) Rather than hiding the Original/Simple/Plain selection, grey them out
> when the first preference means they don't apply.

Can you give some examples where the OriginalHTML/SimpleHTML/PlainText flavors are not applicable?

> And also fix bug 550793.

That's fixed.

Comment 4

[alta88]

it is intended to work like this:
if the selection is Summary or Default Format (and the default format per feed folder pref is Summary), then show the render prefs.  the mimeparser only acts on a message (the Summary for feeds) and never on any web page content.  thus, to make clear to the user that it is not an option to render web pages differently, the options were not displayed.

if it's considered important that a)render prefs should be on the bottom, b)should always show and be grayed out, then this bug should stay open.  otherwise it should be closed.

let's ask bwinton..

Comment 5

[alta88]

a secondary question is whether changing the render pref while viewing a feed message should be allowed.  it may be confusing/unexpected that changing this pref while viewing feeds will apply to mail.  (therefore the original attempt to separate the two).  one option is to show the state, but gray it out for feeds.

note one last item for doc purposes: feed messages are flagged as such, and may be moved to non feed account folders and retain their 'feedness', meaning Tb will know that the message can be toggled web page/summary etc.  but per folder summary/web page is a property of the original feed account folder, so in this case the default is summary.  the global override is useful for changing this if desired.

Comment 6

[Thomas D. (:thomas8)]

alta88, thanks for commenting!

(In reply to alta88 from comment #4)
> it is intended to work like this:
> if the selection is Summary or Default Format (and the default format per
> feed folder pref is Summary), then show the render prefs.  the mimeparser
> only acts on a message (the Summary for feeds) and never on any web page
> content.

All right, I see. Since users know nothing about mime parsers, that's another reason to follow the proposals of Mike Cowperthwaite's comment 2 and at least organize these menuitems according to ux-visual-hierarchy and place the controlling and more context-relevant items on top, so that feed layout controllers (summary vs. web page) come first, followed by rendering flavors (original/simple_html/plaintext) which are functionally subordinate as alta88 points out. I'd also think toggling the feed layout is a more frequent action over changing rendering flavors.

> thus, to make clear to the user that it is not an option to render
> web pages differently, the options were not displayed.

Fair enough. Putting controls in functional order as explained above could reduce the surprise moment of the flavor controls disappearing.
But I tend to agree with Mike's comment 2 that the better way of showing the user that these options aren't applicable in the context of "Web page" would be to disable rather than hide them. That's if we can manage to set the radiocheckmark on "Original HTML" or have no checkmark for that particular case, because we can't have a disabled radiocheck on "plain text" while the web page is effectively rendered as original html.
 
> if it's considered important that a)render prefs should be on the bottom,
> b)should always show and be grayed out, then this bug should stay open.

That's what Mike and I recommend.

Comment 7

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #6)
> (In reply to alta88 from comment #4)
> > if the selection is Summary... then show the render prefs.  the mimeparser
> > only acts on a message (the Summary for feeds) and never on any web page
> > content.
> 
> All right, I see. Since users know nothing about mime parsers, that's
> another reason to follow the proposals of Mike Cowperthwaite's comment 2 and
> at least organize these menuitems according to ux-visual-hierarchy

...and avoid problems of ux-implementation-level.

Comment 8

[Thomas D. (:thomas8)]

(In reply to alta88 from comment #5)
> a secondary question is whether changing the render pref while viewing a
> feed message should be allowed.  it may be confusing/unexpected that
> changing this pref while viewing feeds will apply to mail.  (therefore the
> original attempt to separate the two).  one option is to show the state, but
> gray it out for feeds.

Another option is to do bug 458606 to complete the unfinished "original attempt to separate the two" (mailnews.display.html_as vs. rss.display.html_as):

Bug 458606 - Implement libmime code to read rss message body render prefs (rss.display.html_as: feed body as original|simple html | plain text)

Per Bug 458606 Comment 0, we stopped half-way when implementing this:
> per bug 438429 comment 53.  UI and prefs already set in the patch to that bug.

As a result, the status quo is "useless-UI", because even when we show the HTML rendering flavors for RSS summaries, changing them has no effect whatsoever. So users have to go back to mailnews account, change rendering flavor there, then go back to feed account.

I propose that we use this bug to either
a) remove this useles-UI until it's working,
b) or bandaid-fix to at least toggle the mailnews.display.html_as pref which we actually use for rss
c) or fix it properly by finishing rss.display.html_as (bug 458606)

> note one last item for doc purposes: feed messages are flagged as such, ...
> meaning Tb will know that the message can be toggled web page/summary etc. 

If that's the case, wouldn't that help towards fixing bug 458606?

Comment 9

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #8)
> c) or fix it properly by finishing rss.display.html_as (bug 458606)

btw, bug 458606 currently has 5 duplicates (filed between 2010 and end of 2012) and 7 votes.

Comment 10

[Ben Bucksch (:BenB)]

I agree with Thomas D. (and I think that's the first time ;-) ):
* More important items like summary vs. webpage on top, then subordinate ones that depend on that.
* It should be disabled, not hidden (That is, if it applies to some feeds, but not others - which is the case here I think. If some option applies to mails only, but not to any feed items, it should be hidden for feeds).
* As an affordance on *why* it is hidden and not available - it's not at all obvious to the end user -, I would suggest a explanatory tooltip (ala "Option not available, because ...") which is set/unset at the same time we enable/disable it. That is, if the implementation is not too difficult.

> we can manage to set the radiocheckmark on "Original HTML" or have no checkmark for that
> particular case, because we can't have a disabled radiocheck on "plain text" while the
> web page is effectively rendered as original html.

I agree with the logic, it would be nice to make it like that. But only if it's simple to implement, without touching the mail code. The code for this option is already fairly complex.

All that said, not many users go into menus at all, much less this particular one, much less for feeds specifically.

Comment 11

[alta88]

it sure would be nice to do bug 458606; ben isn't that mimeparser stuff your code ;).  it would be about 5 lines of code to test for feed msg/folder and get the appropriate pref..

aside from bug 458606 being done anytime soon (or at all), it doesn't seem correct to show the prefs disabled for web page (even if all options are unchecked).  if disabled for feed summary, with the correct state checked, that means "cannot change but the pref applies".  for a web page they should be hidden otherwise a false "the pref may apply" impression is given.

my own (not very strong) opinion is the render prefs should go on the bottom, be shown only for summary, and be enabled and work in changing the pref, for both feeds and mail, whether checked in feeds or mail.

Comment 12

[Thomas D. (:thomas8)]

(In reply to Ben Bucksch (:BenB) from comment #10)
> I agree with Thomas D. (and I think that's the first time ;-) ):

:)

Blake, there's an historic opportunity here for you to agree on something on which Ben Bucksch and I agree - that's rare indeed ;) - so it just needs a nod of your head, or alternatively, say yes ;)

Comment 13

[Thomas D. (:thomas8)]

(In reply to alta88 from comment #11)
> it sure would be nice to do bug 458606; ben isn't that mimeparser stuff your
> code ;).  it would be about 5 lines of code to test for feed msg/folder and
> get the appropriate pref..
> 
> aside from bug 458606 being done anytime soon (or at all), it doesn't seem
> correct to show the prefs disabled for web page (even if all options are
> unchecked).  if disabled for feed summary, with the correct state checked,
> that means "cannot change but the pref applies".

I haven't seen any suggestions here to disable HTML rendering pref menu items for feed mode "Summary".

> for a web page they should
> be hidden otherwise a false "the pref may apply" impression is given.

Well, maybe. Otoh, I think disabling is also a good way of saying "pref does not apply". Just hiding them will also confuse users because, as Ben says,
> it's not at all obvious to the end user ... *why* it is hidden
It's kinda strange that for the feed layout which has *most* HTML (Webpage mode), we do *not* offer HTML rendering flavors (for reasons of technical implemtation)... 

Perhaps, as a compromise, we can follow Ben's suggestion of disabling and adding a tooltip on disabled html rendering prefs: "When viewing feeds as Webpage, these options are not applicable for technical reasons."


> my own (not very strong) opinion is
> the render prefs should go on the bottom,

as suggested by Mike, me, and Ben

> be shown only for summary,

that's where Mike, me, and Ben tend to disagree, we'd prefer disabling

> and [for Summary/Default mode] be enabled and work in changing the
> pref, for both feeds and mail, whether checked in feeds or mail.

I think that's comment 8, solution b):
> b) or bandaid-fix to at least toggle the mailnews.display.html_as pref which
>    we actually use for rss

I'm not sure what's best in the long run, either
- share the pref between feeds and mailnews, or
- have to separate prefs as per the original plan of bug bug 458606. Note that the caption of the entire menu reads "View > *Feed* Message Body as", which might seem to suggest (from reading the UI) that this is handled separately for Feeds.

Comment 14

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #13)
> > [Alta88] my own (not very strong) opinion is
> > the render prefs should go on the bottom,
> 
> as suggested by Mike, me, and Ben
> 
> > be shown only for summary,
> 
> that's where Mike, me, and Ben tend to disagree, we'd prefer disabling

read: disabling HTML rendering options for feed mode "Web page"

Comment 15

[Ben Bucksch (:BenB)]

> It's kinda strange that for the feed layout which has *most* HTML (Webpage mode),
> we do *not* offer HTML rendering flavors

That is also why: webpages have *too much* HTML geared to a certain browsing device, with navigation bar and sidebars and whatnot. Reducing the HTML would produce awful results. HTML in emails and feeds has only the actual text, is more semantic and thus works after stripping.

Comment 16

[Ben Bucksch (:BenB)]

> I'm not sure what's best in the long run, either
> - share the pref between feeds and mailnews, or

As said elsewhere, we want separate prefs for mail and feeds, because they are different things and people may well want stripped for mail and original for feeds. I subscribe feeds voluntarily, but not emails.

Comment 17

[alta88]

(In reply to Thomas D. from comment #13)
> (In reply to alta88 from comment #11)
> 
> > for a web page they should
> > be hidden otherwise a false "the pref may apply" impression is given.
> 
> Well, maybe. Otoh, I think disabling is also a good way of saying "pref does
> not apply". Just hiding them will also confuse users because, as Ben says,
> > it's not at all obvious to the end user ... *why* it is hidden

disagree.  showing an unchecked, disabled groups of items leads much more to "why".

> It's kinda strange that for the feed layout which has *most* HTML (Webpage
> mode), we do *not* offer HTML rendering flavors (for reasons of technical
> implemtation)... 
> 
> Perhaps, as a compromise, we can follow Ben's suggestion of disabling and
> adding a tooltip on disabled html rendering prefs: "When viewing feeds as
> Webpage, these options are not applicable for technical reasons."
> 

i feel, strongly, that's just bad.

changing web page rendering issues is out of scope here.  most users expect just that, a web page browsing experience.  and that should be treated like it is in Fx, meaning porting the UI in Fx Options->Content panel.  otherwise, AdblockPlus works really well in Tb.  once content tabs were implemented, anything less is an incomplete implementation.

Comment 18

[Thomas D. (:thomas8)]

useless-ui has just been fixed by bug 866498 following my Bug 866498 Comment 6, so the worst part of this has been addressed as described in my comment 8, solution b):
> b) or bandaid-fix to at least toggle the mailnews.display.html_as pref which
>    we actually use for rss

Comment 19

[Thomas D. (:thomas8)]

Adding details to summary for more clarity

Comment 20

[Thomas D. (:thomas8)]

Attachment: Screenshot1: Proposal for Feed Layout = "Summary" to ensure ux-visual-hierarchy and ux-consistency (compared against current behaviour)

Blake, to ease your ui-review, here's a screenshot comparing current and expected behaviour. Note how the feed-layout pref group is currently pushed around in the submenu when you change from layout="Web page" to layout="Summary". For detailed STR, see my next comment.

Reporter and all commenters on the bug so far agree on the expected behaviour of this proposal (Mike Cowperthwaite, alta88, Ben Bucksch, and me).

Comment 21

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

Comment on attachment 745533 [details]
Screenshot1: Proposal for Feed Layout = "Summary" to ensure ux-visual-hierarchy and ux-consistency (compared against current behaviour)

Since it's a flyout menu, I don't think it matters as much which thing is on top, but I agree that it makes more sense in the proposal than in the current implementation.  ui-r=me.  :)

Comment 22

[Thomas D. (:thomas8)]

STR:

0) Use Daily 23.0a1 (2013-05-03) or higher (to avoid confusion of Bug 866498, recently fixed)
1) Select MailNews account, set View > Message Body As > Original HTML 
2) From Blogs & News Feeds Account, select a feed item in message list
3) Set View > Feed Message Body As > Web Page (and remember the submenu layout)
4) Switch feed layout to View > Feed Message Body As > Summary (and compare the submenu layout against step 3)

Actual Result

(see Screenshot1 of attachment 745533 [details], image 1 -left- and image 2 -middle-)

after 3) For feed layout = "Web Page", submenu looks like this:

o Web page
  Summary
  Default Format

after 4) For feed layout = "Summary", same submenu looks like this:

o Original HTML
  Simple HTML
  Plain text
------------------
  Web page
o Summary
  Default Format

Iow, changing the controlling pref (feed layout) currently causes an unexpected re-arrangement of the submenu:
- controlling pref (feed layout) jumps to the bottom
- subordinate pref (html rendering) reappears at the top

Expected Result

(see Screenshot1 of attachment 745533 [details], image 3 on the right)

after 4) For feed layout = "Summary", submenu should preserve logical hierarchy of prefs, based on 3):

  Web page
o Summary
  Default Format
------------------
o Original HTML
  Simple HTML
  Plain text

Iow, changing the controlling pref (feed layout) should *not* change the submenu layout:
- controlling pref (feed layout) should always stay at the top (as in 3)
- subordinate pref (html rendering) should appear at the bottom (because it only applies when controlling pref (feed layout) is set to "Summary")

Reporter and all commenters on this bug so far agree that this would be correct and expected behaviour for Feed Layout = "Summary" (to comply with ux-visual-hierarchy and ux-consistency; furthermore, to improve accessability).

Comment 23

[Thomas D. (:thomas8)]

(In reply to Blake Winton (:bwinton) from comment #21)
> Comment on attachment 745533 [details]
> Screenshot1: Proposal for Feed Layout = "Summary" to ensure
> ux-visual-hierarchy and ux-consistency (compared against current behaviour)
> 
> Since it's a flyout menu, I don't think it matters as much which thing is on
> top, but I agree that it makes more sense in the proposal than in the
> current implementation.  ui-r=me.  :)

Thanks for rapid ui-review! (In a welcome mid-air collision with detailed description of comment 22 :) )

Sakshi (assignee), you can start working to re-shuffle the menus :)

Comment 24

[Mike Cowperthwaite]

Clearing needinfo. I have nothing to add to the above discussion.

Comment 25

[alta88]

is this being worked on?  the patch is simple, and needs to make Tb24.

if there's no patch by end of week, i will reassign and submit one.

Comment 26

[alta88]

Attachment: patch

Comment 27

[Richard Marti (:Paenglab)]

Comment on attachment 761999 [details] [diff] [review]
patch

This looks the same like the ui proposal which blake gave ui-r=+

so ui-r=me

+              <menuitem id="bodyFeedGlobalWebPage" 

and some following lines have trailing whitespaces. Please can you remove them in code you are touching?

Comment 28

[alta88]

Attachment: updated for checkin

right, just needed an r+ in addition.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/f477e12da3c3