Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UIP-2303 getSelectionStart Util for CTE Cursor Caching #69

Merged
merged 13 commits into from
May 22, 2017

Conversation

kaanaksoy-wk
Copy link
Contributor

Ultimate Problem

In order to implement cursor index caching in CTEs, we need a streamlined function that gives us the selectionStart value for any input type.

The getSelectionStart function in this PR handles inconsistent behavior across browsers—Chrome does not support selectionStart for number and email inputs—while also properly handling text areas.

Solution

  • Create a getSelectionStart function similar to the already existing setSelectionRange utility.
  • Write tests to verify the functionality of the getSelectionStart utility.

Testing Suggestions

  • Verify that tests pass.

Possible Areas of Regression

  • N/A

FYA: @aaronlademann-wf @greglittlefield-wf @clairesarsam-wf @joelleibow-wf @jacehensley-wf

@aviary2-wf
Copy link

Raven

Number of Findings: 0

@codecov-io
Copy link

codecov-io commented May 17, 2017

Codecov Report

Merging #69 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #69      +/-   ##
=========================================
+ Coverage   97.69%   97.7%   +0.02%     
=========================================
  Files          28      28              
  Lines        1380    1387       +7     
=========================================
+ Hits         1348    1355       +7     
  Misses         32      32

@@ -149,3 +149,27 @@ void setSelectionRange(/* TextInputElement | TextAreaElement */Element input, in
throw new ArgumentError.value(input, 'input', 'must be an instance of `TextInputElementBase`, `NumberInputElement` or `TextAreaElement`');
}
}

/// Custom implementation to avoid issues with `selectionStart` when accessing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the Dartlang style guide here. This comment would be more compliant as something like:

Returns the index of the first selected character in [input].

To workaround this chrome bug, returns null for [EmailInputElement] or [NumberInputElement] on Chrome. null will also be returned for any [input] that does not support selectionStart.

/// If `selectionStart` is not supported on the input, `null` will be returned.
///
/// See: <https://bugs.chromium.org/p/chromium/issues/detail?id=324360>
int getSelectionStart(Element input) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a type of InputElement may be more appropriate here instead of Element

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I used Element here is because, from what I understand, TextAreaElement does not inherit from InputElement, but we still want to support it in this function. It is the same reason why setSelectionRange also accepts an Element as its parameter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right, that's an annoying quirk. Looks good then!

@@ -154,31 +154,41 @@ main() {
});
});

group('setSelectionRange', () {
test('throws an ArgumentError if called on an unsupported Element type', () {
group('setSelectionRange and getSelectionStart: ', () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these tests are better suited to separate groups - although they're related methods, the test descriptions aren't as clear as they could be and separated tests would be more atomic/unit-ish. So something that looks like:

group('setSelectionRange', () {
  test('throws an ArgumentError if called on an unsupported Element type', () {
    ...
  });
  ...
});

group('getSelectionStart', () {
  test('returns normally if called on an unsupported Element type', () {
    ...
  });
  ...
});

@clairesarsam-wf
Copy link
Contributor

@kaanaksoy-wk Will +1 once test failures are resolved

@clairesarsam-wf
Copy link
Contributor

+1

@greglittlefield-wf greglittlefield-wf changed the title UIP-2162: getSelectionStart Util for CTE Cursor Caching getSelectionStart Util for CTE Cursor Caching May 18, 2017
@greglittlefield-wf
Copy link
Contributor

I'm going to make this a separate ticket as a subtask of the main ticket, so that the main one can be used for the Web Skin Dart PR.

@rm-astro-wf rm-astro-wf changed the title getSelectionStart Util for CTE Cursor Caching UIP-2303 getSelectionStart Util for CTE Cursor Caching May 18, 2017
@greglittlefield-wf
Copy link
Contributor

greglittlefield-wf commented May 18, 2017

+10

@leviwith-wf
Copy link
Contributor

QA +10

  • Testing instruction
  • Dev +1's
  • Dev/QA +10
  • Unit tests created/updated
  • All unit tests pass
  • Rosie ran/Rosie comment displays expected info
  • Dependency Scan Clean

Merging.

@leviwith-wf leviwith-wf merged commit 1bbbd20 into Workiva:master May 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants