Reworked and cleaned up client-side TabSheet and Accordion. (#12357)

- Added and corrected JavaDocs.
- Deprecated unused public methods.
- Fixed first tab style logic in TabSheet.
- Fixed navigation focus logic in TabSheet.
- Fixed tab width bookkeeping for scrolling TabSheet tabs.
- Renamed private methods and variables for clarity.
- Removed unnecessary or duplicated private methods.
- Reworked some logic to clarify it and to better match my understanding
of what's supposed to happen within those methods.
- Updated some deprecated method calls to use currently recommended
solutions.
- Added and updated regression tests.

7 files changed, 218 insertions, 41 deletions

diff --git a/uitest/reference-screenshots/chrome/TabKeyboardNavigationTest-testFocus_ANY_Chrome__scrolled-right-to-tab-12.png b/uitest/reference-screenshots/chrome/TabKeyboardNavigationTest-testFocus_ANY_Chrome__scrolled-right-to-tab-12.png
index 72eef0f89e..f9e67af2ab 100755
--- a/uitest/reference-screenshots/chrome/TabKeyboardNavigationTest-testFocus_ANY_Chrome__scrolled-right-to-tab-12.png
+++ b/uitest/reference-screenshots/chrome/TabKeyboardNavigationTest-testFocus_ANY_Chrome__scrolled-right-to-tab-12.png
diff --git a/uitest/reference-screenshots/chrome/TabSheetFocusingTest-addAndFocusTabs_ANY_Chrome__tabsAdded.png b/uitest/reference-screenshots/chrome/TabSheetFocusingTest-addAndFocusTabs_ANY_Chrome__tabsAdded.png
index 5d1e181671..8745aa7cc2 100755
--- a/uitest/reference-screenshots/chrome/TabSheetFocusingTest-addAndFocusTabs_ANY_Chrome__tabsAdded.png
+++ b/uitest/reference-screenshots/chrome/TabSheetFocusingTest-addAndFocusTabs_ANY_Chrome__tabsAdded.png
diff --git a/uitest/src/main/java/com/vaadin/tests/components/tabsheet/ScrolledTabSheetHiddenTabsResize.java b/uitest/src/main/java/com/vaadin/tests/components/tabsheet/ScrolledTabSheetHiddenTabsResize.java
new file mode 100644
index 0000000000..6584cd69a5
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/tabsheet/ScrolledTabSheetHiddenTabsResize.java
@@ -0,0 +1,20 @@
+package com.vaadin.tests.components.tabsheet;
+
+import com.vaadin.ui.Label;
+import com.vaadin.ui.TabSheet;
+import com.vaadin.ui.TabSheet.Tab;
+
+public class ScrolledTabSheetHiddenTabsResize extends ScrolledTabSheetResize {
+
+    @Override
+    protected void populate(TabSheet tabSheet) {
+        for (int i = 0; i < 40; i++) {
+            String caption = "Tab " + i;
+            Label label = new Label(caption);
+
+            Tab tab = tabSheet.addTab(label, caption);
+            tab.setClosable(true);
+            tab.setVisible(i % 2 != 0);
+        }
+    }
+}
diff --git a/uitest/src/main/java/com/vaadin/tests/components/tabsheet/ScrolledTabSheetResize.java b/uitest/src/main/java/com/vaadin/tests/components/tabsheet/ScrolledTabSheetResize.java
index c84ba5e3ef..28b37ec4e4 100644
--- a/uitest/src/main/java/com/vaadin/tests/components/tabsheet/ScrolledTabSheetResize.java
+++ b/uitest/src/main/java/com/vaadin/tests/components/tabsheet/ScrolledTabSheetResize.java
@@ -21,6 +21,15 @@ public class ScrolledTabSheetResize extends AbstractTestUI {
         TabSheet tabSheet = new TabSheet();
         tabSheet.setSizeFull();
 
+        populate(tabSheet);
+
+        addComponent(tabSheet);
+        addComponent(new Button("use reindeer", e -> {
+            setTheme("reindeer");
+        }));
+    }
+
+    protected void populate(TabSheet tabSheet) {
         for (int i = 0; i < 20; i++) {
             String caption = "Tab " + i;
             Label label = new Label(caption);
@@ -28,11 +37,6 @@ public class ScrolledTabSheetResize extends AbstractTestUI {
             Tab tab = tabSheet.addTab(label, caption);
             tab.setClosable(true);
         }
-
-        addComponent(tabSheet);
-        addComponent(new Button("use reindeer", e -> {
-            setTheme("reindeer");
-        }));
     }
 
     @Override
diff --git a/uitest/src/test/java/com/vaadin/tests/components/tabsheet/ScrolledTabSheetHiddenTabsResizeTest.java b/uitest/src/test/java/com/vaadin/tests/components/tabsheet/ScrolledTabSheetHiddenTabsResizeTest.java
new file mode 100644
index 0000000000..9e3b5aaf25
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/components/tabsheet/ScrolledTabSheetHiddenTabsResizeTest.java
@@ -0,0 +1,31 @@
+package com.vaadin.tests.components.tabsheet;
+
+import java.util.List;
+
+import org.openqa.selenium.WebElement;
+
+public class ScrolledTabSheetHiddenTabsResizeTest
+        extends ScrolledTabSheetResizeTest {
+
+    @Override
+    public void setup() throws Exception {
+        lastVisibleTabCaption = "Tab 39";
+        super.setup();
+    }
+
+    @Override
+    protected WebElement getFirstHiddenViewable(List<WebElement> tabs) {
+        // every other tab is hidden on server, return the second-to-last tab
+        // before the first one that is visible on client
+        WebElement previous = null;
+        WebElement older = null;
+        for (WebElement tab : tabs) {
+            if (hasCssClass(tab, "v-tabsheet-tabitemcell-first")) {
+                break;
+            }
+            older = previous;
+            previous = tab;
+        }
+        return older;
+    }
+}
diff --git a/uitest/src/test/java/com/vaadin/tests/components/tabsheet/ScrolledTabSheetResizeTest.java b/uitest/src/test/java/com/vaadin/tests/components/tabsheet/ScrolledTabSheetResizeTest.java
index 4f0ae02b43..ee16707c77 100644
--- a/uitest/src/test/java/com/vaadin/tests/components/tabsheet/ScrolledTabSheetResizeTest.java
+++ b/uitest/src/test/java/com/vaadin/tests/components/tabsheet/ScrolledTabSheetResizeTest.java
@@ -7,7 +7,9 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.fail;
 
 import java.io.IOException;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 import org.junit.Test;
 import org.openqa.selenium.By;
@@ -16,10 +18,15 @@ import org.openqa.selenium.WebElement;
 import com.vaadin.testbench.TestBenchElement;
 import com.vaadin.testbench.elements.ButtonElement;
 import com.vaadin.testbench.elements.TabSheetElement;
+import com.vaadin.testbench.elements.UIElement;
 import com.vaadin.tests.tb3.MultiBrowserTest;
 
 public class ScrolledTabSheetResizeTest extends MultiBrowserTest {
 
+    protected String lastVisibleTabCaption = "Tab 19";
+
+    private WebElement pendingTab = null;
+
     @Override
     public void setup() throws Exception {
         super.setup();
@@ -29,13 +36,14 @@ public class ScrolledTabSheetResizeTest extends MultiBrowserTest {
     @Test
     public void testReindeer() throws IOException, InterruptedException {
         $(ButtonElement.class).first().click();
+        Map<String, Integer> sizes = saveWidths();
         StringBuilder exceptions = new StringBuilder();
         boolean failed = false;
         // upper limit is determined by the amount of tabs,
         // lower end by limits set by Selenium version
         for (int i = 1400; i >= 650; i = i - 50) {
             try {
-                testResize(i);
+                testResize(i, sizes);
             } catch (Exception e) {
                 if (failed) {
                     exceptions.append(" --- ");
@@ -61,13 +69,14 @@ public class ScrolledTabSheetResizeTest extends MultiBrowserTest {
 
     @Test
     public void testValo() throws IOException, InterruptedException {
+        Map<String, Integer> sizes = saveWidths();
         StringBuilder exceptions = new StringBuilder();
         boolean failed = false;
         // 1550 would be better for the amount of tabs (wider than for
         // reindeer), but IE11 can't adjust that far
         for (int i = 1500; i >= 650; i = i - 50) {
             try {
-                testResize(i);
+                testResize(i, sizes);
             } catch (Exception e) {
                 if (failed) {
                     exceptions.append(" --- ");
@@ -91,10 +100,56 @@ public class ScrolledTabSheetResizeTest extends MultiBrowserTest {
         }
     }
 
-    private void testResize(int start)
+    private Map<String, Integer> saveWidths() {
+        // save the tab widths before any scrolling
+        TabSheetElement ts = $(TabSheetElement.class).first();
+        Map<String, Integer> sizes = new HashMap<>();
+        for (WebElement tab : ts
+                .findElements(By.className("v-tabsheet-tabitemcell"))) {
+            if (hasCssClass(tab, "v-tabsheet-tabitemcell-first")) {
+                // skip the first visible for now, it has different styling and
+                // we are interested in the non-styled width
+                pendingTab = tab;
+                continue;
+            }
+            if (pendingTab != null && tab.isDisplayed()) {
+                String currentLeft = tab.getCssValue("padding-left");
+                String pendingLeft = pendingTab.getCssValue("padding-left");
+                if (currentLeft == null || "0px".equals(currentLeft)) {
+                    currentLeft = tab.findElement(By.className("v-caption"))
+                            .getCssValue("margin-left");
+                    pendingLeft = pendingTab
+                            .findElement(By.className("v-caption"))
+                            .getCssValue("margin-left");
+                }
+                if (currentLeft != pendingLeft && currentLeft.endsWith("px")
+                        && pendingLeft.endsWith("px")) {
+                    WebElement caption = pendingTab
+                            .findElement(By.className("v-captiontext"));
+                    sizes.put(caption.getAttribute("innerText"),
+                            pendingTab.getSize().getWidth()
+                                    - intValue(pendingLeft)
+                                    + intValue(currentLeft));
+                }
+                pendingTab = null;
+            }
+            WebElement caption = tab.findElement(By.className("v-captiontext"));
+            sizes.put(caption.getAttribute("innerText"),
+                    tab.getSize().getWidth());
+        }
+        return sizes;
+    }
+
+    private Integer intValue(String pixelString) {
+        return Integer
+                .valueOf(pixelString.substring(0, pixelString.indexOf("px")));
+    }
+
+    private void testResize(int start, Map<String, Integer> sizes)
             throws IOException, InterruptedException {
-        testBench().resizeViewPortTo(start, 600);
+        resizeViewPortTo(start);
         waitUntilLoadingIndicatorNotVisible();
+        sleep(100); // a bit more for layouting
 
         int iterations = 0;
         while (scrollRight() && iterations < 50) {
@@ -102,28 +157,35 @@ public class ScrolledTabSheetResizeTest extends MultiBrowserTest {
             ++iterations;
         }
 
-        // FIXME: TabSheet definitely still has issues,
-        // but it's moving to a better direction.
-
-        // Sometimes the test never realises that scrolling has
-        // reached the end, but this is not critical as long as
-        // the other criteria is fulfilled.
-        // If we decide otherwise, uncomment the following check:
-        // if (iterations >= 50) {
-        // fail("scrolling right never reaches the end");
-        // }
-
-        // This fails on some specific widths by ~15-20 pixels, likewise
-        // deemed as non-critical for now so commented out.
-        // assertNoExtraRoom(start);
+        if (iterations >= 50) {
+            fail("scrolling right never reaches the end");
+        }
+        assertNoExtraRoom(start, sizes);
 
-        testBench().resizeViewPortTo(start + 150, 600);
+        resizeViewPortTo(start + 150);
         waitUntilLoadingIndicatorNotVisible();
+        sleep(100); // a bit more for layouting
 
-        assertNoExtraRoom(start + 150);
+        assertNoExtraRoom(start + 150, sizes);
     }
 
-    private void assertNoExtraRoom(int width) {
+    private void resizeViewPortTo(int width) {
+        try {
+            testBench().resizeViewPortTo(width, 600);
+        } catch (UnsupportedOperationException e) {
+            // sometimes this exception is thrown even if resize succeeded, test
+            // validity
+            waitUntilLoadingIndicatorNotVisible();
+            UIElement ui = $(UIElement.class).first();
+            int currentWidth = ui.getSize().width;
+            if (currentWidth != width) {
+                // only throw the exception if the size didn't change
+                throw e;
+            }
+        }
+    }
+
+    private void assertNoExtraRoom(int width, Map<String, Integer> sizes) {
         TabSheetElement ts = $(TabSheetElement.class).first();
         WebElement scroller = ts
                 .findElement(By.className("v-tabsheet-scroller"));
@@ -131,19 +193,35 @@ public class ScrolledTabSheetResizeTest extends MultiBrowserTest {
                 .findElements(By.className("v-tabsheet-tabitemcell"));
         WebElement lastTab = tabs.get(tabs.size() - 1);
 
-        assertEquals("Tab 19",
+        assertEquals("Unexpected last visible tab,", lastVisibleTabCaption,
                 lastTab.findElement(By.className("v-captiontext")).getText());
 
+        WebElement firstHidden = getFirstHiddenViewable(tabs);
+        if (firstHidden == null) {
+            // nothing to scroll to
+            return;
+        }
+        // the sizes change during a tab's life-cycle, use the recorded size
+        // approximation for how much extra space adding this tab would need
+        // (measuring a hidden tab would definitely give too small width)
+        WebElement caption = firstHidden
+                .findElement(By.className("v-captiontext"));
+        String captionText = caption.getAttribute("innerText");
+        Integer firstHiddenWidth = sizes.get(captionText);
+        if (firstHiddenWidth == null) {
+            firstHiddenWidth = sizes.get("Tab 3");
+        }
+
         int tabWidth = lastTab.getSize().width;
         int tabRight = lastTab.getLocation().x + tabWidth;
-        int scrollerLeft = scroller.findElement(By.tagName("button"))
-                .getLocation().x;
+        assertThat("Unexpected tab width", tabRight, greaterThan(20));
 
-        assertThat("Not scrolled to the end (width: " + width + ")",
-                scrollerLeft, greaterThan(tabRight));
-        // technically this should probably be just greaterThan,
+        int scrollerLeft = scroller.getLocation().x;
+        // technically these should probably be just greaterThan,
         // but one pixel's difference is irrelevant for now
-        assertThat("Too big gap (width: " + width + ")", tabWidth,
+        assertThat("Not scrolled to the end (width: " + width + ")",
+                scrollerLeft, greaterThanOrEqualTo(tabRight));
+        assertThat("Too big gap (width: " + width + ")", firstHiddenWidth,
                 greaterThanOrEqualTo(scrollerLeft - tabRight));
     }
 
@@ -163,4 +241,18 @@ public class ScrolledTabSheetResizeTest extends MultiBrowserTest {
         }
     }
 
+    /*
+     * There is no way to differentiate between hidden-on-server and
+     * hidden-on-client here, so this method has to be overridable.
+     */
+    protected WebElement getFirstHiddenViewable(List<WebElement> tabs) {
+        WebElement previous = null;
+        for (WebElement tab : tabs) {
+            if (hasCssClass(tab, "v-tabsheet-tabitemcell-first")) {
+                break;
+            }
+            previous = tab;
+        }
+        return previous;
+    }
 }
diff --git a/uitest/src/test/java/com/vaadin/tests/components/tabsheet/TabSheetFocusedTabTest.java b/uitest/src/test/java/com/vaadin/tests/components/tabsheet/TabSheetFocusedTabTest.java
index b8921bd0a7..15059fd324 100644
--- a/uitest/src/test/java/com/vaadin/tests/components/tabsheet/TabSheetFocusedTabTest.java
+++ b/uitest/src/test/java/com/vaadin/tests/components/tabsheet/TabSheetFocusedTabTest.java
@@ -24,22 +24,53 @@ public class TabSheetFocusedTabTest extends MultiBrowserTest {
 
         getTab(1).click();
 
-        assertTrue(isFocused(getTab(1)));
+        assertTrue("Tab 1 should have been focused but wasn't.",
+                isFocused(getTab(1)));
 
         new Actions(getDriver()).sendKeys(Keys.ARROW_RIGHT).perform();
 
-        assertFalse(isFocused(getTab(1)));
-        assertTrue(isFocused(getTab(3)));
+        assertFalse("Tab 1 was focused but shouldn't have been.",
+                isFocused(getTab(1)));
+        assertTrue("Tab 3 should have been focused but wasn't.",
+                isFocused(getTab(3)));
 
         getTab(5).click();
 
-        assertFalse(isFocused(getTab(3)));
-        assertTrue(isFocused(getTab(5)));
+        assertFalse("Tab 3 was focused but shouldn't have been.",
+                isFocused(getTab(3)));
+        assertTrue("Tab 5 should have been focused but wasn't.",
+                isFocused(getTab(5)));
 
         getTab(1).click();
 
-        assertFalse(isFocused(getTab(5)));
-        assertTrue(isFocused(getTab(1)));
+        assertFalse("Tab 5 was focused but shouldn't have been.",
+                isFocused(getTab(5)));
+        assertTrue("Tab 1 should have been focused but wasn't.",
+                isFocused(getTab(1)));
+    }
+
+    @Test
+    public void scrollingChangesFocusedTab() {
+        openTestURL();
+
+        getTab(7).click();
+
+        assertTrue("Tab 7 should have been focused but wasn't.",
+                isFocused(getTab(7)));
+
+        findElement(By.className("v-tabsheet-scrollerNext")).click();
+
+        assertFalse("Tab 7 was focused but shouldn't have been.",
+                isFocused(getTab(7)));
+        assertTrue("Tab 3 should have been focused but wasn't.",
+                isFocused(getTab(3)));
+
+        new Actions(getDriver()).sendKeys(Keys.ARROW_RIGHT).perform();
+
+        assertFalse("Tab 3 was focused but shouldn't have been.",
+                isFocused(getTab(3)));
+        assertTrue("Tab 5 should have been focused but wasn't.",
+                isFocused(getTab(5)));
     }
 
     private WebElement getTab(int index) {
@@ -49,7 +80,6 @@ public class TabSheetFocusedTabTest extends MultiBrowserTest {
     }
 
     private boolean isFocused(WebElement tab) {
-
         return tab.getAttribute("class").contains("v-tabsheet-tabitem-focus");
     }