[MGNLUI-2624] Show popup with hidden tabs, if there are too many tabs to fit in one row Created: 23/Jan/14  Updated: 10/Sep/15  Resolved: 18/Aug/15

Status: Closed
Project: Magnolia UI
Component/s: user interaction, widgets
Affects Version/s: 5.2
Fix Version/s: 5.3.11, 5.4.2

Type: Improvement Priority: Neutral
Reporter: Mikaël Geljić Assignee: Aleksandr Pchelintcev
Resolution: Fixed Votes: 1
Labels: support, unizh, ux
Remaining Estimate: 0d
Time Spent: 17.75d
Original Estimate: 0.25d

Attachments: PNG File 1 Choose arrow.png     PNG File 2 Choose hidden tab.png     PNG File 3 Tab chosen.png     PNG File qa-sample-dialog-with-long-tweaked-titles.png     PNG File shop3.png    
Issue Links:
Cloners
clones MGNLUI-2364 Save/Cancel buttons disappear if dial... Closed
dependency
duplicate
is duplicated by MGNLUI-3057 If sub app tabs need more than one li... Closed
is duplicated by MGNLUI-3305 Pages App: Tabs in lower row can not ... Closed
is duplicated by MGNLUI-3308 Dialog tabs are shown in 3 rows inste... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Date of First Response:
Sprint: Sprint 2 (Basel), Sprint 6 (Basel)
Story Points: 1

 Description   

When tabsheet holds more tabs than its width can afford, the extra tabs overflow to a second row, where they were never designed to go.

We're going to implement the "Safari approach" to fix this problem:

  • The last tab is a combined tab w drop-down
    • A click on the dropdown icon OR the already selected tab opens the dropdown, which shows all currently hidden tabs.
    • When resizing the browser or the dialog, the hidden tabs should be calculated again

To switch between tabs using the keyboard, you use the arrow keys to change the focus to a different tab, then switch to it by e.g. pressing SPACE. This is mentioned here to complete the concept, but should be treated in a separate follow-up issue (MGNLUI-3452) .



 Comments   
Comment by Andreas Weder [ 03/Mar/14 ]

I've attached screenshots showing the visual design for this.

Please note, though, that the popup shown does not only show the hidden tabs (marked in black), but all tabs. We only want to show the hidden tabs.

Comment by Sang Ngo Huu [ 01/Jun/15 ]

I've chosen the approach: show both visible tabs and hidden tabs on popup. Like eclipse approach. I think it is easy for users to have a look all tabs

The code has been checked into MGNLUI-2624_sn branch.

Comment by Mikaël Geljić [ 02/Jun/15 ]

Please stick to the specified approach in ticket description—we've reached consensus in UX quite some time ago on that one.

Comment by Sang Ngo Huu [ 04/Jun/15 ]

I created MGNLUI-3452 to follow up the concept as description.

Comment by Mikaël Geljić [ 05/Jun/15 ]

Reverted, text description to original; I see where the confusion came from, so I'll clarify if I may:

Attached screenshots merely show the visual design, not the behavior, while text description was then different, with respect to displaying (or not displaying) non-hidden tabs in the popup.

By the way, it's also not showing a "real" dialog (no "Show all" tab), but the "asset chooser", a feature which is yet to come, so again, these were attached mostly to highlight the look of the popup, not its content.

—Sorry again about that confusion.

Comment by Aleksandr Pchelintcev [ 14/Jul/15 ]

sang.ngo I took over this issue after reviewing, cause it looked like quite a lot of things had to be simplified/changed/removed. Here's what I've found:

  • ResizeEvent
    • What was the need of this event?
    • You have got a tabsheet view at your disposal and can instruct it to resize, instead an artificial event is added:
      • its name does not specify that it is related to tabsheet
      • for some reason it drags an active tab-label as an argument, why? - resizing does not affect current active tab, so why dispatching it?
    • Decision - remove this event, create method MagnoliaTabSheetView#onResize() which explicitly resizes the tab bar.
  • TabClickedEvent
    • Also do not see the purpose for this event
    • We already have a mechanism to dispatch the active tab changes
    • DomEvent.fireNativeEvent(Document.get().createClickEvent(0, 0, 0, 0, 0, false, false, false, false), moreTabs); - what was the purpose of this line of code.
    • Decision - event was removed with the handlers and its impementations
  • CaptionChangedEvent
    • Name also does not indicate relation to TabShee by any means
    • Why you do that in the first place? the only usage I see is in the TabBarWidget which already has all the tab labels and can directly query the captions from those.
    • Decision - remove this event and handlers
  • CollectionUtil#reserveItemToFirst
    • Method name does not reflect what method really does
    • If it is not possible to set a self-explaining method name - pls add at least JavaDoc
    • Anyway, the logic of the method sounds like an overkill - backwards iteration starting from teh active tab should be enough
  • TabBarWidget
    • Why keeping everything in the maps (visibleTabs, tabSizes, menuItems)?
    • It increases complexity - you have to make sure maps are up to date all the time, whereas all the required information is in the field objects, why duplicating it?
    • moreTabs is a bad name for a class field, as well as VShellMoreTabLabel is a bad class name
    • VShellMoreTabLabel -> HiddenTabsPopup and it now does not extend a SimplePanel (there are no widget to attach, why should it be a panel??)
    • calculateHiddenTabs - also not very descriptive method name -> reArrangeTabVisibility
    • also as discussed above - I removed all the handlers of the new events
  • CSS
    • Since we use SASS, we now can assign symbolic aliases to values like color, font-size etc. Pls use those whenever possible when specifying colors (see colors.scss).
Comment by Mikaël Geljić [ 29/Jul/15 ]

There's a bunch of minor issues with the MagnoliaTabSheet actually (regardless of this feature), which I'm currently trying to address while we're at it:

  • Adding tab programmatically is buggy, results in stacking new tab content below active tab content
  • Consequently, hidden tabs toggle is not "re-positioned" when adding/removing tabs. (It is however repositioned when setting "show all" on/off).
  • Popup has limited width (150px max, not sure why), and may be positioned outside viewport.
  • Hidden tab toggle should probably -stick to the right and- have no border
Comment by Mikaël Geljić [ 12/Aug/15 ]

I pushed to new branch feature/tabsheet-hidden-tabs-mge with the following fixes:

General tabsheet consolidation/QA:

  • Fix default visibility of new tabs content—otherwise all tabs content components would stack upon one another
  • Prevent from binding event handlers multiple times - onLoad() will be called every time upon attach (and we don't unbind onUnload), but those handlers don't rely on physical attachment, so they may be bound upon construct.
  • Keep show-all tab last in the DOM when inserting new tabs
  • Make eventBus final

Hidden-tabs popup feature improvements:

  • Styling fixes: font color for popup, remove inline styles, fix text ellipsis
  • Append hidden tabs to DOM in ctor, and make it final
  • No need to cast as LIElement
  • Make HiddenTabsPopup a nested class, pass-in eventBus dependency, avoid cross-self-referencing through field
  • Re-iterate over tab-reordering logic
    • Separate tab-row updating part vs. popup-content updating part, they may well differ so this makes it easier to maintain

Not done:

  • Fix absolute positioning (could still be positioned outside viewport for "edge" cases)
  • Add current "toggle-able" tab to the mix and highlight it in the popup (to better convey tabs natural ordering)
  • Still glitches on tab-bar in some edge conditions
    • tab might go to next-line until next resize, only experienced when adding tabs dynamically, which is not our case besides my test sandbox
    • shows more tabs than it can host, only experienced in such narrow cases that only one tab can fit
  • For the app viewport, close/maximize icons may overlap, especially since they are absolute so we barely have any control over that from the tabsheet perspective.
Comment by Mikaël Geljić [ 12/Aug/15 ]

Last but not least, UI test adjustments are required. This especially shows in fields' UI tests, with the "tab-heavy" dialog showroom.
Reopening for once... / postponement it is.

ComplexFieldUITest.testMultiFieldWithCustomPropertyBuilder:103 Cannot execute click on non existing WebElement: By.xpath: //*[contains(@class, 'v-shell-tabsheet')]//*[@class = 'tab-title' and text() = 'Multi Fields']

ComplexFieldUITest.testMultiFieldWithDefaultPropertyBuilderAddRemove:51 Cannot execute click on non existing WebElement: By.xpath: //*[contains(@class, 'v-shell-tabsheet')]//*[@class = 'tab-title' and text() = 'Multi Fields']

CompositeFieldUITest.testI18nCompositeField:74 Cannot execute click on non existing WebElement: By.xpath: //*[contains(@class, 'v-shell-tabsheet')]//*[@class = 'tab-title' and text() = 'Switch']

MultiFieldUITest.testI18nMultiField:77 Cannot execute click on non existing WebElement: By.xpath: //*[contains(@class, 'v-shell-tabsheet')]//*[@class = 'tab-title' and text() = 'Multi Fields']
Comment by Aleksandr Pchelintcev [ 13/Aug/15 ]

mgeljic Re: UI tests. Thanks to the fact that we use fancy methods to access the common UI parts and tabs in particular (e.g. AbstractMagnoliaUITest#getTabForCaption) I think we could relatively easily modify the tab resolution method, smth like this:

    protected WebElement getTabForCaption(String tabCaption) {
        WebElement tabElement = getElementByXpath("//*[contains(@class, 'v-shell-tabsheet')]//*[@class = 'tab-title' and text() = '%s']", tabCaption);
        if (!isExisting(tabElement)) {
            final WebElement popupControl = getElementByXpath("//*[contains(@class, 'v-shell-tabsheet')]//*[contains(@class, 'hidden-tabs-popup-button')]");
            popupControl.click();
            tabElement = getElementByXpath("//*[contains(@class, 'context-menu-wrapper')]//*[@class = 'menubar']//*[contains(@class, 'menu-item') and text() = '%s']", tabCaption);

        }
        return tabElement;
    }
Comment by Aleksandr Pchelintcev [ 18/Aug/15 ]

Updated UI tests can be found at a branch tabsheet-changes-ui-tests-adaptation

Comment by Andreas Weder [ 10/Sep/15 ]

Ok. We don't have gamification in Jira. I really think we should find a plug-in that allows us to like certain events and to give kudos to others.

Because I just wanted to say a big THANK YOU to everyone involved in this issue. It's been quite some work to get this done. And it was worth it: this feature solves a long-standing issue, but also has a visual and behavioral quality to it, which makes me . Thanks again.

Generated at Mon Feb 12 08:58:32 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.