1943358 - notion.so - "Reposition" feature for images does not work

Status: NEW

(Web Compatibility :: Site Reports, defect, P2)

Description

[Calin Tanase]

Attachment: image reposition FF vs Chrome.mp4

Environment:
Operating system: Linux / Windows 10
Firefox version: Firefox 134.0 (release) / 136

Preconditions:

• Account needed (login with Google account available)
• Clean profile

Steps to reproduce:

1. Navigate to: https://www.notion.so/
2. Log in with your account.
3. Go to an existing page (or create one by clicking on the button "Create a new page" next to your account name).
4. Hover above the page title.
5. Click on "Add cover".
6. Hover over the cover and click "Reposition".
7. Click and drag down the cover and release.
8. Observe the behavior.

Expected Behavior:
The cover position changes and can be saved.

Actual Behavior:
The cover position resets and editing mode cancels.

Notes:

• Reproducible on the latest Firefox Release and Nightly
• Reproducible regardless of the ETP setting
• Works as expected using Chrome

---

Created from webcompat-user-report:9c0163eb-a3e8-4c23-b759-c25bb1a3cc1a

Comment 1

[BugBot [:suhaib / :marco]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

Comment 2

[Sean Feng [:sefeng211]]

I haven't found the root cause of this so far..still looking

Comment 3

[Sean Feng [:sefeng211]]

Upon the mouseup event handling, the site executes these code in Firefox.

          (
            () => function (e, t, o) {
              return (0, c().hj) (o) ? o : function (e, t) {
                if ('gallery' === e || 'board' === e) {
                  const e = t.getFormat().card_cover_position;
                  if ((0, c().hj) (e)) return e
                }
                const o = 0.5;
                if (t.isCollectionView()) {
                  const e = t.getCollectionViewCollectionStore();
                  if (e) {
                    const t = e.getFormat().collection_cover_position;
                    return (0, p().z4) (t) ? o : t
                  }
                }
                const n = t.getFormat(),
                r = n.page_cover_position;
                if ((0, c().hj) (r)) return r;
                return o
              }(e, t)
            }(S, x, d.temporaryPosition)

That const o = 0.5; is the key here, basically the above code will reset the object-position property for this image to 50%.

By comparing the execution, the code differs at this function

        const h = function(e) {
            const t = (0,
            n.useRef)(null)
              , r = (0,
            i().O7)()
              , {isMobile: s} = (0,
            i().Fy)()
              , [d,u] = (0,
            n.useState)(f)
              , g = (0,
            n.useCallback)(( () => {
                u(f)
            }
            ), [])
              , {renderControl: h, onFlushTemporaryPosition: v, style: b, source: S, store: x} = e
              , {isRepositioning: C, image: k, startDragRenderedPosition: P, startDragImageScreenHeight: w} = d
              , M = (0,
            a().VK)(( () => 0 === o(965903).default.state.stores.length), [])
              , [_,I] = (0,
            n.useState)(!1)
              , T = (0,
            a().qz)(e.mouseStore, o(233417).Z)
              , R = (0,

It's the default.state.stores.length value is different between Firefox and Chrome. It was 1 in Firefox and 0 in Chrome, that caused the callstack becomes different, and Chrome didn't execute the function that reset its position.

Comment 4

[Daniel Holbert [:dholbert]]

For reference, the JS in comment 3 is here, for me at least:
https://www.notion.so/_assets/73155-2def67dbe976f2b2.js

I spent a little while poking around to see if I could learn more about why o(965903).default.state.stores.length is different when mouseup arrives, but it's non-trivial... I did find that o here seems to be some sort of registry of various objects with numeric IDs, and the 965903 object-id is only passed in a few places. In particular, the places I found were: the spot quoted in comment 3, and here:

      z = () => o(965903),

https://www.notion.so/_assets/floatingTableOfContents-4211eca2e9c2929d.js
(so in that file, all z() usages are referring to this same object with the all-important default.state.stores.length value)

...and in a bunch of spots in this file (with different single-character aliases similar to the z one above):
https://www.notion.so/_assets/91852-b680287d52efaae9.js

I'm not immediately finding where/why that object's default.state.stores array is going from empty to non-empty, though.

Comment 5

[Sean Feng [:sefeng211]]

Thanks Daniel!

I think we've spent enough time to diagnosis bug, I'll just put this into the blocked queue.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

We tried debugging this during our needs diagnosis meeting. We didn't get that far.

I've reached out to Notion but haven't heard back yet.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

The .default.state.stores.length code looks more like this now:

        i = (0, n(484714).v3) (),
        {
          renderControl: a,
          onFlushTemporaryPosition: s,
          style: d,
          source: p,
          store: y
        }
        = e,
        v = (0, r().K8) ((() => 0 === n(502438).default.state.stores.length), []),

Comment 8

[Jeff Muizelaar [:jrmuizel]]

It looks like stores is filled with code around here:

                   if ((0, v().K) (o)) {
                      var r;
                      let e = (0, y().T) (o.start.store);
                      if (
                        (null === (r = e) || void 0 === r ? void 0 : r.getType()) === s().xd.tableRow
                      ) {
                        var a;
                        const t = null === (a = e.getRecordStoreUIParent()) ||
                        void 0 === a ? void 0 : a.recordStoreUIParentStore;
                        t &&
                        t instanceof m().B &&
                        (e = t)
                      }
                      if (!e) return;
                      return {
                        stores: [
                          e
                        ]
                      }

Comment 9

[Jeff Muizelaar [:jrmuizel]]

And that seems to be coming from code having to do with selection:

                                A('onSelect: delegateSelection', t),
                                t.leaf.delegateSelection('start')

Comment 10

[Jeff Muizelaar [:jrmuizel]]

There's this code:

if (A("onSelect (react selection)", d().toStaticRange(n)),
                !ne.current && n && ue.current && n.commonAncestorContainer === ue.current)

Chrome seems to be getting true for n.commonAncestorContainer === ue.current and we get false.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

n is a selection range and n.commonAncestorContainer is <div class=layout> in Firefox but matches ue.current in Chrome which is a small <span>

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Attachment: Potentially reduced test-case.

Dragging over the image doesn't change the selection in chrome, but it focuses the toplevel editor and collapses the selection to start in Firefox.

I think that at least explains the selection change Jeff is seeing.

Comment 13

[Emilio Cobos Álvarez [:emilio]]

The first selection change comes from:

#12 0x00007f2d8c78d0ad in mozilla::SelectionChangeEventDispatcher::OnSelectionChange (this=this@entry=0x7f2d827e8f20, aDoc=0x7f2d830d5200, aSel=aSel@entry=0x7f2d9c14fbc0, aReason=0)
    at /home/emilio/src/moz/firefox-2/dom/base/SelectionChangeEventDispatcher.cpp:77
#13 0x00007f2d8c787e1f in mozilla::dom::Selection::NotifySelectionListeners (this=this@entry=0x7f2d9c14fbc0) at /home/emilio/src/moz/firefox-2/dom/base/Selection.cpp:4064
#14 0x00007f2d8c77fcd5 in mozilla::dom::Selection::RemoveAllRangesInternal (this=0x7f2d9c14fbc0, aRv=<optimized out>, aIsUnlinking=<optimized out>) at /home/emilio/src/moz/firefox-2/dom/base/Selection.cpp:2573
#15 0x00007f2d90d6353d in nsFrameSelection::ClearNormalSelection (this=this@entry=0x7f2d9c14f780) at /home/emilio/src/moz/firefox-2/layout/generic/nsFrameSelection.cpp:2186
#16 0x00007f2d90d66771 in nsFrameSelection::SetAncestorLimiter (this=0x7f2d9c14f780, aLimiter=<optimized out>) at /home/emilio/src/moz/firefox-2/layout/generic/nsFrameSelection.cpp:3083
#17 0x00007f2d8c786a58 in mozilla::dom::Selection::SetAncestorLimiter (this=<optimized out>, aLimiter=0x7f2d812045e0) at /home/emilio/src/moz/firefox-2/dom/base/Selection.cpp:2312
#18 0x00007f2d908ee2e2 in mozilla::HTMLEditor::InitializeSelectionAncestorLimit (this=0x7f2d82744800, aAncestorLimit=...) at /home/emilio/src/moz/firefox-2/editor/libeditor/HTMLEditor.cpp:1093
#19 0x00007f2d9084d2c4 in mozilla::EditorBase::InitializeSelection (this=0x7f2d82744800, aOriginalEventTargetNode=<optimized out>) at /home/emilio/src/moz/firefox-2/editor/libeditor/EditorBase.cpp:5798
#20 0x00007f2d908693cb in mozilla::EditorBase::OnFocus (this=0x2, aOriginalEventTargetNode=...) at /home/emilio/src/moz/firefox-2/editor/libeditor/EditorBase.cpp:6204
#21 0x00007f2d908ec782 in mozilla::HTMLEditor::OnFocus (this=0x7f2d82744800, aOriginalEventTargetNode=...) at /home/emilio/src/moz/firefox-2/editor/libeditor/HTMLEditor.cpp:795
#22 0x00007f2d9088c9fb in mozilla::EditorEventListener::Focus (this=this@entry=0x7f2d82738f70, aFocusEvent=<optimized out>) at /home/emilio/src/moz/firefox-2/editor/libeditor/EditorEventListener.cpp:1171
#23 0x00007f2d9088bb63 in mozilla::EditorEventListener::HandleEvent (this=this@entry=0x7f2d82738f70, aEvent=aEvent@entry=0x7f2d846b5f90) at /home/emilio/src/moz/firefox-2/editor/libeditor/EditorEventListener.cpp:449
#24 0x00007f2d9095c335 in mozilla::HTMLEditorEventListener::HandleEvent (this=0x7f2d82738f70, aEvent=0x7f2d846b5f90) at /home/emilio/src/moz/firefox-2/editor/libeditor/HTMLEditorEventListener.cpp:102
#25 0x00007f2d8e5c7922 in mozilla::EventListenerManager::HandleEventSingleListener (this=this@entry=0x7f2d83a1aca0, aListener=<optimized out>,
    aListener@entry=0x7f2d8467af58, aTypeAtom=aTypeAtom@entry=0x7f2d86cfde84 <mozilla::detail::gGkAtoms+73436>, aEvent=aEvent@entry=0x7ffe2d07d8a0, aDOMEvent=0x7f2d846b5f90, aCurrentTarget=0x7f2d9c132b80, aItemInShadowTree=<optimized out>)
    at /home/emilio/src/moz/firefox-2/dom/events/EventListenerManager.cpp:1273

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Masayuki, thoughts about our behaviour vs. Chrome's on the test case?

Comment 15

[Emilio Cobos Álvarez [:emilio]]

I confirmed that a patch like this:

diff --git a/editor/libeditor/EditorBase.cpp b/editor/libeditor/EditorBase.cpp
index ab31cb916f665..27a1da98d3981 100644
--- a/editor/libeditor/EditorBase.cpp
+++ b/editor/libeditor/EditorBase.cpp
@@ -5765,6 +5765,8 @@ nsresult EditorBase::InitializeSelection(
     return NS_ERROR_FAILURE;
   }

+  return NS_OK;
+
   // Init the caret
   RefPtr<nsCaret> caret = GetCaret();
   if (NS_WARN_IF(!caret)) {

Fixes the notion issue (of course it causes other issues elsewhere).

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sounds like Chrome does not make the editing host get focused when it does not have focus and mouse down on non-editable element. If it's right, I guess EventStateManager::PostHandleEvent sets focus to the editing host and modifying here will fix this bug. Although I don't have much time for now because this is a risky change and may need follow up work for regressions.

Comment 17

[Emilio Cobos Álvarez [:emilio]]

Chrome does seem to focus it tho, there's a focusring around the editable element. It's just the caret doesn't get moved.