Closed Bug 954467 Opened 10 years ago Closed 10 years ago

Scrolling to the first unread message is too difficult

Categories

(Instantbird Graveyard :: Conversation, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(1 file, 6 obsolete files)

*** Original post on bio 1032 at 2011-09-14 10:54:00 UTC ***

Ideally the first unread message would be halfway down the screen, so some context is visible.
Severity: normal → enhancement
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1032 as attmnt 1029 at 2011-11-26 17:39:00 UTC ***

How about this?

Should this be optional? To me it is the expected behaviour (show me the first unread message on restoring from hold).
Attachment #8352771 - Flags: review?(clokep)
*** Original post on bio 1032 at 2011-11-26 18:16:20 UTC ***

(In reply to comment #1)

> Should this be optional? To me it is the expected behaviour (show me the first
> unread message on restoring from hold).

Should it be an add-on? It's almost never the behavior I expect:
- if the conversation is restored automatically because someone has pinged me, it's that ping that I want to see.
- if I'm restoring a conversation because I want to say something, I want to see current messages / what has just been said to ensure I'm not interrupting, not old messages.
- if I'm restoring a conversation with hundreds of unread messages, I'm unlikely to read them all. I'll probably scroll up a little, but not to the first unread message.
- if I'm restoring a conversation with only a handful of unread messages, they are most likely all shown already when the scrollbar is at the bottom.
*** Original post on bio 1032 at 2011-11-26 18:47:07 UTC ***

(In reply to comment #2)
> Should it be an add-on? It's almost never the behavior I expect:

Always interesting to see how expectations differ :)

> - if the conversation is restored automatically because someone has pinged me,
> it's that ping that I want to see.

I agree this case would have to be treated differently. Not hard to fix.

> - if I'm restoring a conversation because I want to say something, I want to
> see current messages / what has just been said to ensure I'm not interrupting,
> not old messages.
> - if I'm restoring a conversation with hundreds of unread messages, I'm
> unlikely to read them all. I'll probably scroll up a little, but not to the
> first unread message.
> - if I'm restoring a conversation with only a handful of unread messages, they
> are most likely all shown already when the scrollbar is at the bottom.

I suppose it's a personal preference whether one prefers to scroll to the bottom when necessary (which is easy to do) as opposed to scroll up to the first unread message when that is what is wanted (which is much harder to do and takes longer). Depends how often you end up doing one thing or the other...

But clearly it's no good as a default if it's not a common preference.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1032 as attmnt 1030 at 2011-11-26 19:04:00 UTC ***

Fix: Do not scroll if pinged.
Attachment #8352772 - Flags: review?(clokep)
Comment on attachment 8352771 [details] [diff] [review]
Patch

*** Original change on bio 1032 attmnt 1029 at 2011-11-26 19:04:06 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352771 - Attachment is obsolete: true
Attachment #8352771 - Flags: review?(clokep)
*** Original post on bio 1032 at 2011-11-26 19:24:08 UTC ***

(In reply to comment #3)
> scroll up to the
> first unread message when that is what is wanted (which is much harder to do
> and takes longer).

Isn't this the frustration you are trying to avoid, and so the real bug? "Scrolling to the first unread is too hard and takes too long." (I suspect it's even almost impossible without reading the text of the messages if the message theme used doesn't support context messages.)

Maybe we should think about how this could be improved?

I think on Mac I would like the swipe-up gesture on the touchpad to scroll to the first unread message rather than the top of the context part of the conversation. Obviously that doesn't help you though :-/.
*** Original post on bio 1032 at 2011-11-26 19:27:44 UTC ***

(In reply to comment #5)
> Obviously that doesn't help you though :-/.

Hmm, maybe we could find a cross platform keyboard shortcut? I'm afraid that wouldn't be very discoverable, but it would at least stop our own frustration.

What about something like shift+begin? (shift + page up already scrolls the history while the textbox is foxued)
*** Original post on bio 1032 at 2011-11-26 20:46:47 UTC ***

(In reply to comment #5)
> Should it be an add-on?

Thinking about it a bit, it seems very hard to do as an add-on, certainly not without some ugly hacks, because 1) you'd want to catch conversation-loaded, but by design conversation-loaded is fired before the existing messages are displayed and 2) you'd have to jump through hoops to get the position to scroll to.

> Isn't this the frustration you are trying to avoid, and so the real bug?
> "Scrolling to the first unread is too hard and takes too long." 

This is definitely the key frustration. But what's the desired default behaviour for each person still seems dependent on one's usage pattern. For example from personal experience, if one prefers conversation windows which are relatively small (as opposed to almost maximized) this ("if I'm restoring a conversation with only a handful of unread messages, they are most likely all shown already when the scrollbar is at the bottom") is almost never the case.

> (I suspect it's even almost impossible without reading the text of the 
> messages if the message theme used doesn't support context messages.)

Yep, another reason one should consider message styles without context message support to be broken.

> What about something like shift+begin? (shift + page up already scrolls the
history while the textbox is foxued)

I like the idea of a keyboard shortcut/swipe gesture. Shift-End is used frequently to select up to the end of the text; Shift-Home selects to the beginning. It's less frequently used but I thought I'd mention it as it makes it less discoverable. Ctrl/Cmd-Home? 

Would it be worth it having the behaviour of the patch as an option? (Possibly even per about:config only.) I suppose the difficult question to answer is "how many people would prefer it that way"?
*** Original post on bio 1032 at 2011-11-26 21:55:59 UTC ***

(In reply to comment #7)
> (In reply to comment #5)
> > Should it be an add-on?
> 
> Thinking about it a bit, it seems very hard to do as an add-on, certainly not
> without some ugly hacks, because 1) you'd want to catch conversation-loaded,
> but by design conversation-loaded is fired before the existing messages are
> displayed and 2) you'd have to jump through hoops to get the position to scroll
> to.

I would prefer if we could avoid using the technical difficulty of doing something as an add-on based on the current code as an argument while discussing which default behavior the UI should have.
If the current code makes something difficult to extend, it's a bug in itself, regardless of whether we want to change the behavior.

> Shift-End is used
> frequently to select up to the end of the text; Shift-Home selects to the
> beginning.

Uh, I forgot that, sorry.

> It's less frequently used but I thought I'd mention it as it makes
> it less discoverable. Ctrl/Cmd-Home?

Ctrl+Home sounds ok then.
By the way, Cmd-Home isn't really relevant as MacBook keyboards don't have Home/End keys (the OS supports them for desktop / external keyboard though). For my personal usage, the swipe gesture is better anyway :).
 

> Would it be worth it having the behaviour of the patch as an option? (Possibly
> even per about:config only.)

I dislike options. ;)

Also, note that if the required code to scroll to the right place when a keyboard shortcut is pressed is added, then calling the same code from an add-on should be trivial.

> I suppose the difficult question to answer is "how many people would prefer it that way"?

If we can implement something that feels like an improvement and that doesn't get in the way of anybody, then only one user for the feature is enough for it to be acceptable.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1032 as attmnt 1031 at 2011-11-26 23:45:00 UTC ***

Based on the discussion, reorganized the patch. 

Alt-PgUp now scrolls to the first unread message, Alt-PgDn scrolls to the bottom (last message). If there are no context messages Alt-PgUp scrolls to the top. (Surprisingly no keyboard shortcuts for this already existed; I think it fits well with the existing Shift-PgUp/Dn).

(Ctrl+Home and Ctrl+End are used in the textbox to jump to the beginning of the first line/end of the last line respectively, so are no good. Ctrl+PgUp/PgDn switch between tabs.)


(In reply to comment #8)
> For my personal usage, the swipe gesture is better anyway :).

I don't know how to add that, nor would I be able to test it - but it should be trivial to add now.

> I dislike options. ;)

Me too :) But the question is what's the default that fits the most people, and how does one change it (add-on or something else). I have no idea.

> If we can implement something that feels like an improvement and that doesn't
> get in the way of anybody, then only one user for the feature is enough for it
> to be acceptable.

:)
Attachment #8352773 - Flags: review?(clokep)
Comment on attachment 8352772 [details] [diff] [review]
Patch

*** Original change on bio 1032 attmnt 1030 at 2011-11-26 23:45:50 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352772 - Attachment is obsolete: true
Attachment #8352772 - Flags: review?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1032 as attmnt 1032 at 2011-11-26 23:47:00 UTC ***

Typo in comments.
Attachment #8352774 - Flags: review?(clokep)
Comment on attachment 8352773 [details] [diff] [review]
Patch

*** Original change on bio 1032 attmnt 1031 at 2011-11-26 23:47:31 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352773 - Attachment is obsolete: true
Attachment #8352773 - Flags: review?(clokep)
*** Original post on bio 1032 at 2011-11-27 00:47:21 UTC ***

(In reply to comment #9)

> (In reply to comment #8)
> > For my personal usage, the swipe gesture is better anyway :).
> 
> I don't know how to add that

See http://lxr.instantbird.org/instantbird/source/instantbird/content/macgestures.js#185

By the way, it seems we have two many different methods for scrolling already. Wouldn't it make sense to concentrate them inside the convbrowser.xml binding?
*** Original post on bio 1032 at 2011-11-27 12:04:05 UTC ***

(In reply to comment #11)
> By the way, it seems we have two many different methods for scrolling already.
> Wouldn't it make sense to concentrate them inside the convbrowser.xml binding?

Yes, the scrollTo method should be moved to convbrowser.xml in a final version of this patch.
Comment on attachment 8352774 [details] [diff] [review]
Patch

*** Original change on bio 1032 attmnt 1032 at 2011-11-27 12:18:32 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352774 - Flags: review?(clokep)
*** Original post on bio 1032 at 2011-11-27 12:30:19 UTC ***

I'm going to rewrite/reorganize this so pressing Alt+PgUp twice jumps to the top top of the messages, i.e. Alt+PgUp/Dn scroll to the previous/next 'section'.
*** Original post on bio 1032 at 2011-11-27 18:05:01 UTC ***

For the record, Alt-PgUp/Dn is also broken with this implementation after the window is resized, as the Y position to scroll to will change.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1032 as attmnt 1033 at 2011-11-28 02:23:00 UTC ***

Completely reworked patch. Note this patch does not scroll to the first unread message by default on restoring from hold. 

Instead it implements Alt-PgUp/PgDn (and the corresponding mouse gestures) to scroll to the previous/next "section". E.g. for PgUp this means: to the first unread (non-Context) message when below it, otherwise to the top. The unread message is displayed half-way down the window to provide context.

There remains the question of what the preferred default behaviour for restoring a conversation from hold should be and/or how to change it (add-on? config option?)
Attachment #8352775 - Flags: review?(clokep)
Comment on attachment 8352774 [details] [diff] [review]
Patch

*** Original change on bio 1032 attmnt 1032 at 2011-11-28 02:23:26 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352774 - Attachment is obsolete: true
*** Original post on bio 1032 at 2011-11-28 12:21:57 UTC ***

(In reply to comment #15)
> Created attachment 8352775 [details] [diff] [review] (bio-attmnt 1033) [details]
> Patch
> 
> Completely reworked patch.

I haven't reviewed this patch, just quickly looked at it, but I wanted to note that I like this approach much better! :)

> There remains the question of what the preferred default behaviour for
> restoring a conversation from hold should be and/or how to change it (add-on?
> config option?)

Add-on :).
Summary: Scroll to first unread message when opening hidden conversation → Scrolling to the first unread message is too difficult
*** Original post on bio 1032 at 2011-11-29 02:22:40 UTC ***

(In reply to comment #15)
> Created attachment 8352775 [details] [diff] [review] (bio-attmnt 1033) [details]
> Patch
> 
> Completely reworked patch. Note this patch does not scroll to the first unread
> message by default on restoring from hold. 
This patch doesn't apply for me at all. I think you gave a diff between the rewrite and a previous iteration of the patch?
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1032 as attmnt 1035 at 2011-11-29 11:49:00 UTC ***

(In reply to comment #17)
> This patch doesn't apply for me at all. I think you gave a diff between the
> rewrite and a previous iteration of the patch?

No, it's complete. A path was wrong though.
Attachment #8352777 - Flags: review?(clokep)
Comment on attachment 8352775 [details] [diff] [review]
Patch

*** Original change on bio 1032 attmnt 1033 at 2011-11-29 11:49:46 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352775 - Attachment is obsolete: true
Attachment #8352775 - Flags: review?(clokep)
Comment on attachment 8352777 [details] [diff] [review]
Patch

*** Original change on bio 1032 attmnt 1035 at 2011-11-29 13:01:04 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352777 - Flags: review?(clokep) → review+
Attachment #8352777 - Flags: review?(florian)
*** Original post on bio 1032 at 2011-11-29 13:01:04 UTC ***

Comment on attachment 8352777 [details] [diff] [review] (bio-attmnt 1035)
Patch

>diff -u /chat/content/convbrowser.xml ./convbrowser.xml
>--- /chat/content/convbrowser.xml	2011-11-27 14:39:56.000000000 +0100
>+++ ./convbrowser.xml	2011-11-28 03:01:45.000000000 +0100
>@@ -279,7 +279,8 @@
>       </method>
>
>       <field name="_lastMessage">null</field>
>-      <field name="_lastMessageIsContext">false</field>
>+      <field name="_lastMessageIsContext">true</field>
>+      <field name="_firstNoncontextElt">null</field>
>
>       <method name="appendMessage">
>         <parameter name="aMsg"/>
>@@ -329,11 +330,43 @@
>               this._scrollToElement(newElt);
>             this._lastElement = newElt;
>             this._lastMessage = aMsg;
>+            if (this._lastMessageIsContext != aContext)
>+              this._firstNoncontextElt = newElt;
>             this._lastMessageIsContext = aContext;
>           ]]>
>         </body>
>       </method>
>
>+      <method name="scrollToPrevSection">
>+        <body>
>+        <![CDATA[
>+          let noncontextY = (this._firstNoncontextElt ? this._firstNoncontextElt.offsetTop : 0);
>+          if (this.contentWindow.scrollY < noncontextY)
>+            this.contentWindow.scrollTo(0, 0);
>+          else {
>+            this.contentWindow.scrollTo(0,
>+              Math.max(noncontextY - Math.round(this.clientHeight/2), 0));
>+          }
>+          this._updateAutoScrollEnabled();
>+        ]]>
>+        </body>
>+      </method>
>+
>+      <method name="scrollToNextSection">
>+        <body>
>+        <![CDATA[
>+          let maxY = this.contentWindow.scrollMaxY;
>+          let noncontextY = (this._firstNoncontextElt ? this._firstNoncontextElt.offsetTop : maxY);
>+          let sectionY = Math.min(noncontextY - Math.round(this.clientHeight/2), maxY);
>+          if (this.contentWindow.scrollY >= sectionY)
>+            this.contentWindow.scrollTo(0, maxY);
>+          else
>+            this.contentWindow.scrollTo(0, sectionY);
>+          this._updateAutoScrollEnabled();
>+        ]]>
>+        </body>
>+      </method>
>+
>       <method name="browserScroll">
>         <parameter name="event"/>
>         <body>
>@@ -790,7 +823,8 @@
>           // are swapped.
>           var fieldsToSwap = ["_docShell", "_fastFind", "_contentWindow", "_webNavigation",
>                               "_theme", "_textModifiers", "_autoScrollEnabled",
>-                              "_lastElement", "_lastMessage", "_lastMessageIsContext"];
>+                              "_lastElement", "_lastMessage", "_lastMessageIsContext",
>+                              "_firstNoncontextElt"];
>
>           var ourFieldValues = {};
>           var otherFieldValues = {};
>diff -u /instantbird/content/conversation.xml ./conversation.xml
>--- /instantbird/content/conversation.xml	2010-01-01 00:00:00.000000000 +0100
>+++ ./conversation.xml	2011-11-28 02:44:52.000000000 +0100
>@@ -471,6 +471,18 @@
>           return;
>         }
>
>+        if (event.altKey && event.keyCode == KeyEvent.DOM_VK_PAGE_UP) {
>+          this.browser.scrollToPrevSection();
>+          event.preventDefault();
>+          return;
>+        }
>+
>+        if (event.altKey && event.keyCode == KeyEvent.DOM_VK_PAGE_DOWN) {
>+          this.browser.scrollToNextSection();
>+          event.preventDefault();
>+          return;
>+        }
>+
>         var inputBox = this.editor;
>         // When attempting to copy an empty selection, copy the
>         // browser selection instead (see bug 954128 (bio 693)).
>diff -u /instantbird/content/macgestures.js ./macgestures.js
>--- /instantbird/content/macgestures.js	2011-11-28 03:13:03.000000000 +0100
>+++ ./macgestures.js	2011-11-28 03:11:41.000000000 +0100
>@@ -185,12 +185,12 @@
>       case "swipe-down":
>         if (aEvent.originalTarget.ownerDocument == getBrowser().contentDocument)
>           getBrowser().contentWindow.focus();
>-        goDoCommand("cmd_scrollBottom");
>+        getBrowser().selectedBrowser.scrollToNextSection();
>         break;
>       case "swipe-up":
>         if (aEvent.originalTarget.ownerDocument == getBrowser().contentDocument)
>           getBrowser().contentWindow.focus();
>-        goDoCommand("cmd_scrollTop");
>+        getBrowser().selectedBrowser.scrollToPrevSection();
>         break;
>       case "swipe-left":
>       case "swipe-right":
*** Original post on bio 1032 at 2011-11-29 13:07:23 UTC ***

Uhhh...I'm not sure why it just put the whole patch in comment and destroyed my actual comment I wrote. :(

But I said it looked good and I agree it should be in an add-on (and might need to be configurable by MUC). The only comment I had is that I liked the formatting of scrollToNextSection better than scrollToPrevSection:
>+          let sectionY = Math.min(noncontextY - Math.round(this.clientHeight/2), maxY);
>+          if (this.contentWindow.scrollY >= sectionY)
>+            this.contentWindow.scrollTo(0, maxY);
>+          else
>+            this.contentWindow.scrollTo(0, sectionY);

instead of 
>+          if (this.contentWindow.scrollY < noncontextY)
>+            this.contentWindow.scrollTo(0, 0);
>+          else {
>+            this.contentWindow.scrollTo(0,
>+              Math.max(noncontextY - Math.round(this.clientHeight/2), 0));
>+          }

But that I would let flo comment on this.

Also the Mac gesture needs testing.
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Hardware: x86 → All
*** Original post on bio 1032 at 2011-11-29 13:37:29 UTC ***

(In reply to comment #20)
> The only comment I had is that I liked the
> formatting of scrollToNextSection better than scrollToPrevSection:

Just to note that it isn't just different formatting - the condition is slightly different in a way that the formatting reflects.
*** Original post on bio 1032 at 2011-11-29 13:46:54 UTC ***

(In reply to comment #21)
> (In reply to comment #20)
> > The only comment I had is that I liked the
> > formatting of scrollToNextSection better than scrollToPrevSection:
> 
> Just to note that it isn't just different formatting - the condition is
> slightly different in a way that the formatting reflects.

No, it is different formatting. The condition is different, yes; but I'm referring to precalculating sectionY vs. calculating it inline. I understand it's only used once in the latter case, but I think it's cleaner to do it beforehand, as in the former case.
Attached patch PatchSplinter Review
*** Original post on bio 1032 as attmnt 1036 at 2011-11-29 14:22:00 UTC ***

(In reply to comment #22)
> No, it is different formatting. The condition is different, yes; but I'm
> referring to precalculating sectionY vs. calculating it inline. I understand
> it's only used once in the latter case, but I think it's cleaner to do it
> beforehand, as in the former case.

Right. It even saves a line ;) (at the cost of a local var)
Attachment #8352778 - Flags: review?(florian)
Comment on attachment 8352777 [details] [diff] [review]
Patch

*** Original change on bio 1032 attmnt 1035 at 2011-11-29 14:22:15 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352777 - Attachment is obsolete: true
Attachment #8352777 - Flags: review?(florian)
Comment on attachment 8352778 [details] [diff] [review]
Patch

*** Original change on bio 1032 attmnt 1036 at 2011-12-13 23:21:01 UTC ***

This looks good overall, and I look forward to using it in the next nightly!

During review I only found very trivial coding style nits and a parse error, so I fixed them myself.

>diff -u /chat/content/convbrowser.xml ./convbrowser.xml

>+      <field name="_firstNoncontextElt">null</field>

* _firstNoncontextElt -> _firstNonContextElt


>+      <method name="scrollToPrevSection">

* scrollToPrevSection -> scrollToPreviousSection

>+          let noncontextY = (this._firstNoncontextElt ? this._firstNoncontextElt.offsetTop : 0);

* This line is too long, it can be wrapped after the =.
* () isn't necessary here.
* noncontextY -> nonContextY

>+          let sectionY = Math.max(noncontextY - Math.round(this.clientHeight/2), 0));

* Trailing ) causes a parse error.
* clientHeight/2 -> clientHeight / 2
Attachment #8352778 - Flags: review?(florian) → review+
*** Original post on bio 1032 at 2011-12-13 23:24:49 UTC ***

https://hg.instantbird.org/instantbird/rev/8d564c726cfb
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
Depends on: 954643
No longer depends on: 954643
You need to log in before you can comment on or make changes to this bug.