Fix a couple mobile bugs on the unified banner's review menu.

Review Request #14130 — Created Aug. 29, 2024 and submitted — Latest diff uploaded

Information

Review Board
release-7.x

Reviewers

We had a couple bugs when using the review menu on mobile.

The first has to do with the event overlay that we add to the page when the
review menu opens. The overlay wasn't going away when closing the menu by
tapping somewhere else on the unified banner (i.e. tapping the mode selector,
publish buttons or empty space). And, if you opened the review menu again, a
new overlay would be added, and the overlays would keep stacking if you
repeated this.

This is fixed by listening to the closing event on the menu and removing
the overlay that way. Previously, we subclassed the closeMenu()
method that exists on the parent Actions.MenuActionView and put the
overlay removal logic in there. However that method doesn't actually
always get called when the menu closes. The grandparent Ink MenuView
sometimes closes the menu directly through its close() method. We may
want to remove or rework these Actions.MenuActionView.openMenu() and
closeMenu() methods in the future, since it could lead to unexpected
state leftover like it did here.

Secondly, the contents of the menu would get cut off when viewing on
mobile. This change makes things look much better by forcing the menu
contents to take up the entire width of the screen and wrap any text
in it.

  • Tested opening, closing, and selecting items in the review menu in
    mobile and desktop.
  • Tested exiting the review menu by it and a lot of other places.
  • Tested scrolling with the menu open.
  • Ran JS unit tests.

Diff Revision 3 (Latest)

orig
1
2
3

Commits

First Last Summary ID Author
Fix a couple mobile bugs on the unified banner's review menu.
We had a couple bugs when using the review menu on mobile. The first has to do with the event overlay that we add to the page when the review menu opens. The overlay wasn't going away when closing the menu by tapping somewhere else on the unified banner (i.e. tapping the mode selector, publish buttons or empty space). And, if you opened the review menu again, a new overlay would be added, and the overlays would keep stacking if you repeated this. This is fixed by listening to the closing event on the menu and removing the overlay that way. Previously, we subclasses the `closeMenu()` method that exists on the parent `Actions.MenuActionView` and put the overlay removal logic in there. However that method doesn't actually always get called when the menu closes. The grandparent Ink `MenuView` sometimes closes the menu directly through its `close()` method. We may want to remove or rework these `Actions.MenuActionView.openMenu()` and `closeMenu()` methods in the future, since it could lead to unexpected state leftover like it did here. Secondly, the contents of the menu would get cut off when viewing on mobile. This change fixes this by forcing the menu contents to take up the width of the screen and wrap any text in it.
62e2ea7bdcb15660fc0043d9652c041b4f024dfc Michelle Aubin

Files

reviewboard/static/rb/js/reviews/views/reviewRequestActions.ts
reviewboard/static/rb/js/reviews/views/reviewRequestActions.ts
Revision 065a7d6cec5da8eeb8cba9ee7f37dd1412d3f8fd New Change
501 lines
502

    
   
502

   
503
    /** The event overlay when the menu is shown in mobile mode. */
503
    /** The event overlay when the menu is shown in mobile mode. */
504
    #overlay: OverlayView = null;
504
    #overlay: OverlayView = null;
505

    
   
505

   
506
    /**
506
    /**
507
     * Close the menu.
507
     * Render the view.
508
     */
508
     */
509
    protected closeMenu() {
509
    protected onInitialRender() {
510
        super.closeMenu();
510
        super.onInitialRender();
511

    
   
511

   
512 Moved to line 576
        if (this.#overlay) {
512
        this.listenTo(this.menu, 'closing', this._removeOverlay);
513
            this.#overlay.remove();

   
514
            this.#overlay = null;

   
515
        }

   
516
    }
513
    }
517

    
   
514

   
518
    /**
515
    /**
519
     * Handle a touchstart event.
516
     * Handle a touchstart event.
520
     *
517
     *
521
     * Args:
518
     * Args:
522
     *     e (TouchEvent):
519
     *     e (TouchEvent):
523
     *         The touch event.
520
     *         The touch event.
524
     */
521
     */
525
    protected onTouchStart(e: TouchEvent) {
522
    protected onTouchStart(e: TouchEvent) {
526
        super.onTouchStart(e);
523
        super.onTouchStart(e);
527

    
   
524

   
528
        if (this.menu.isOpen) {
525
        if (this.menu.isOpen) {

    
   
526
            if (!this.#overlay) {
529
            this.#overlay = new OverlayView();
527
>>>>            this.#overlay = new OverlayView();
530
            this.#overlay.$el.appendTo('body');
528
>>>>            this.#overlay.$el.appendTo('body');
531

    
   
529

   
532
            this.listenTo(this.#overlay, 'click', () => {
530
>>>>            this.listenTo(this.#overlay, 'click', () => {
533
                this.closeMenu();
531
>>>>                this.closeMenu();
534
            });
532
>>>>            });
535
        }
533
>>>>        }
536
    }
534
>>>>    }
537
}
535
>>>>}
538

    
   
536

   

    
   
537
    /**

    
   
538
     * Position the menu.

    
   
539
     *

    
   
540
     * Version Added:

    
   
541
     *     7.0.3

    
   
542
     */

    
   
543
    protected positionMenu() {

    
   
544
        const $menuEl = this.menu.$el;

    
   
545

   

    
   
546
        if (RB.PageManager.getPage().inMobileMode) {

    
   
547
            /*

    
   
548
             * Make the review menu take up the full width of the screen

    
   
549
             * when on mobile.

    
   
550
             *

    
   
551
             * This needs to happen before the call to the parent class so

    
   
552
             * that the parent uses the updated width for the menu.

    
   
553
             */

    
   
554
            $menuEl.css({

    
   
555
                'text-wrap': 'wrap',

    
   
556
                width: $(window).width(),

    
   
557
            });

    
   
558
        } else {

    
   
559
            /* Use default styling on desktop. */

    
   
560
            $menuEl.css({

    
   
561
                'text-wrap': '',

    
   
562
                width: '',

    
   
563
            });

    
   
564
        }

    
   
565

   

    
   
566
        super.positionMenu();

    
   
567
    }

    
   
568

   

    
   
569
    /**

    
   
570
     * Remove the event overlay that's shown in mobile mode.

    
   
571
     *

    
   
572
     * Version Added:

    
   
573
     *     7.0.3

    
   
574
     */

    
   
575
    private _removeOverlay() {

    
   
576 Moved from line 512
        if (this.#overlay) {

    
   
577
            this.#overlay.remove();

    
   
578
            this.#overlay = null;

    
   
579
        }

    
   
580
    }

    
   
581
}

    
   
582

   
539

    
   
583

   
540
/**
584
/**
541
 * Action view for the "Add File" command.
585
 * Action view for the "Add File" command.
542
 *
586
 *
543
 * Version Added:
587
 * Version Added:
213 lines