From 3f25928380b36834e0cbe249938581ede9dc3c6e Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Tue, 2 May 2017 16:34:39 +0100 Subject: [PATCH 1/5] Fix jumping to an unread event when in MELS This adds the `data-contained-scroll-tokens` API to elements in `ScrollPanel` which allows arbitrary containers of elements with scroll tokens to declare their contained scroll tokens. When jumping to a scroll token inside a container, the `ScrollPanel` will act as if it is scrolling to the container itself, not the child. MELS has been modified such that it exposes the scroll tokens of all events that exist within it.This means "Jump to unread message" will work if the unread event is within a MELS (which is any member event, because even individual member events surrounded by other events are put inside a MELS). --- src/components/structures/MessagePanel.js | 1 - src/components/structures/ScrollPanel.js | 5 +++++ src/components/views/elements/MemberEventListSummary.js | 5 +++-- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index 246f351841..74aec61511 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -354,7 +354,6 @@ module.exports = React.createClass({ {eventTiles} diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index d43e22e2f1..da3b303d89 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -551,6 +551,11 @@ module.exports = React.createClass({ var messages = this.refs.itemlist.children; for (var i = messages.length-1; i >= 0; --i) { var m = messages[i]; + if (m.dataset.containedScrollTokens && + m.dataset.containedScrollTokens.indexOf(scrollToken) !== -1) { + node = m; + break; + } if (!m.dataset.scrollToken) continue; if (m.dataset.scrollToken == scrollToken) { node = m; diff --git a/src/components/views/elements/MemberEventListSummary.js b/src/components/views/elements/MemberEventListSummary.js index 63bd2a7c39..a24e19577d 100644 --- a/src/components/views/elements/MemberEventListSummary.js +++ b/src/components/views/elements/MemberEventListSummary.js @@ -369,6 +369,7 @@ module.exports = React.createClass({ render: function() { const eventsToRender = this.props.events; + const eventIds = eventsToRender.map(e => e.getId()); const fewEvents = eventsToRender.length < this.props.threshold; const expanded = this.state.expanded || fewEvents; @@ -379,7 +380,7 @@ module.exports = React.createClass({ if (fewEvents) { return ( -
+
{expandedEvents}
); @@ -437,7 +438,7 @@ module.exports = React.createClass({ ); return ( -
+
{toggleButton} {summaryContainer} {expanded ?
 
: null} From fe83a99ab709121d2259b25d1a735babb2a022ee Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Tue, 2 May 2017 17:36:59 +0100 Subject: [PATCH 2/5] Update ScrollPanel docs --- src/components/structures/ScrollPanel.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index da3b303d89..7fa320d784 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -50,6 +50,10 @@ if (DEBUG_SCROLL) { * serialise the scroll state, and returned as the 'trackedScrollToken' * attribute by getScrollState(). * + * Child elements that contain elements that have scroll tokens must declare the + * contained scroll tokens using 'data-contained-scroll-tokens`. When scrolling + * to a contained scroll token, the ScrollPanel will scroll to the container. + * * Some notes about the implementation: * * The saved 'scrollState' can exist in one of two states: From 4febc63aeecee7613681749b3aa435cbd25ea17f Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Tue, 2 May 2017 17:41:09 +0100 Subject: [PATCH 3/5] Add comment to _scrollToToken --- src/components/structures/ScrollPanel.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 7fa320d784..3f36fac89b 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -555,6 +555,9 @@ module.exports = React.createClass({ var messages = this.refs.itemlist.children; for (var i = messages.length-1; i >= 0; --i) { var m = messages[i]; + // 'data-contained-scroll-tokens' has been set, indicating that a child + // element contains elements that each have a token. Check this array of + // tokens for `scrollToken`. if (m.dataset.containedScrollTokens && m.dataset.containedScrollTokens.indexOf(scrollToken) !== -1) { node = m; From bfa3123f9ba7f3aeab19c7205123c4639f7cd1ea Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 4 May 2017 10:00:13 +0100 Subject: [PATCH 4/5] Combine data-scroll-token and -contained-scroll-tokens - Instead of using one attribute, use one that might just contain one token - Use the first token when tracking a child - Mandate that no commas can be in individual tokens --- src/components/structures/MessagePanel.js | 2 +- src/components/structures/ScrollPanel.js | 41 ++++++++----------- .../views/elements/MemberEventListSummary.js | 6 +-- .../views/rooms/SearchResultTile.js | 2 +- .../components/structures/ScrollPanel-test.js | 2 +- 5 files changed, 24 insertions(+), 29 deletions(-) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index 74aec61511..d4bf147ad5 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -472,7 +472,7 @@ module.exports = React.createClass({ ret.push(
  • + data-scroll-tokens={scrollToken}> excessHeight) { break; @@ -423,7 +423,8 @@ module.exports = React.createClass({ * scroll. false if we are tracking a particular child. * * string trackedScrollToken: undefined if stuckAtBottom is true; if it is - * false, the data-scroll-token of the child which we are tracking. + * false, the fist token in data-scroll-tokens of the child which we are + * tracking. * * number pixelOffset: undefined if stuckAtBottom is true; if it is false, * the number of pixels the bottom of the tracked child is above the @@ -555,16 +556,10 @@ module.exports = React.createClass({ var messages = this.refs.itemlist.children; for (var i = messages.length-1; i >= 0; --i) { var m = messages[i]; - // 'data-contained-scroll-tokens' has been set, indicating that a child - // element contains elements that each have a token. Check this array of - // tokens for `scrollToken`. - if (m.dataset.containedScrollTokens && - m.dataset.containedScrollTokens.indexOf(scrollToken) !== -1) { - node = m; - break; - } - if (!m.dataset.scrollToken) continue; - if (m.dataset.scrollToken == scrollToken) { + // 'data-scroll-tokens' is a DOMString of comma-separated scroll tokens + // There might only be one scroll token + if (m.dataset.scrollTokens && + m.dataset.scrollTokens.split(',').indexOf(scrollToken) !== -1) { node = m; break; } @@ -580,7 +575,7 @@ module.exports = React.createClass({ var boundingRect = node.getBoundingClientRect(); var scrollDelta = boundingRect.bottom + pixelOffset - wrapperRect.bottom; - debuglog("ScrollPanel: scrolling to token '" + node.dataset.scrollToken + "'+" + + debuglog("ScrollPanel: scrolling to token '" + scrollToken + "'+" + pixelOffset + " (delta: "+scrollDelta+")"); if(scrollDelta != 0) { @@ -603,12 +598,12 @@ module.exports = React.createClass({ for (var i = messages.length-1; i >= 0; --i) { var node = messages[i]; - if (!node.dataset.scrollToken) continue; + if (!node.dataset.scrollTokens) continue; var boundingRect = node.getBoundingClientRect(); newScrollState = { stuckAtBottom: false, - trackedScrollToken: node.dataset.scrollToken, + trackedScrollToken: node.dataset.scrollTokens.split(',')[0], pixelOffset: wrapperRect.bottom - boundingRect.bottom, }; // If the bottom of the panel intersects the ClientRect of node, use this node @@ -620,7 +615,7 @@ module.exports = React.createClass({ break; } } - // This is only false if there were no nodes with `node.dataset.scrollToken` set. + // This is only false if there were no nodes with `node.dataset.scrollTokens` set. if (newScrollState) { this.scrollState = newScrollState; debuglog("ScrollPanel: saved scroll state", this.scrollState); diff --git a/src/components/views/elements/MemberEventListSummary.js b/src/components/views/elements/MemberEventListSummary.js index a24e19577d..ae8678894d 100644 --- a/src/components/views/elements/MemberEventListSummary.js +++ b/src/components/views/elements/MemberEventListSummary.js @@ -369,7 +369,7 @@ module.exports = React.createClass({ render: function() { const eventsToRender = this.props.events; - const eventIds = eventsToRender.map(e => e.getId()); + const eventIds = eventsToRender.map(e => e.getId()).join(','); const fewEvents = eventsToRender.length < this.props.threshold; const expanded = this.state.expanded || fewEvents; @@ -380,7 +380,7 @@ module.exports = React.createClass({ if (fewEvents) { return ( -
    +
    {expandedEvents}
    ); @@ -438,7 +438,7 @@ module.exports = React.createClass({ ); return ( -
    +
    {toggleButton} {summaryContainer} {expanded ?
     
    : null} diff --git a/src/components/views/rooms/SearchResultTile.js b/src/components/views/rooms/SearchResultTile.js index 7fac244481..1aba7c9196 100644 --- a/src/components/views/rooms/SearchResultTile.js +++ b/src/components/views/rooms/SearchResultTile.js @@ -60,7 +60,7 @@ module.exports = React.createClass({ } } return ( -
  • +
  • {ret}
    • ); }, diff --git a/test/components/structures/ScrollPanel-test.js b/test/components/structures/ScrollPanel-test.js index eacaeb5fb4..7ecb74be6f 100644 --- a/test/components/structures/ScrollPanel-test.js +++ b/test/components/structures/ScrollPanel-test.js @@ -115,7 +115,7 @@ var Tester = React.createClass({ // // there is an extra 50 pixels of margin at the bottom. return ( -
  • +
  • {key} From 6d9a1f047d1a2a82d3d0ab9596a30eafafb5c626 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 4 May 2017 13:03:04 +0100 Subject: [PATCH 5/5] Typo --- src/components/structures/ScrollPanel.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 50951312e7..a652bcc827 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -423,7 +423,7 @@ module.exports = React.createClass({ * scroll. false if we are tracking a particular child. * * string trackedScrollToken: undefined if stuckAtBottom is true; if it is - * false, the fist token in data-scroll-tokens of the child which we are + * false, the first token in data-scroll-tokens of the child which we are * tracking. * * number pixelOffset: undefined if stuckAtBottom is true; if it is false,