fix(form-field): FormField with outline doesn't work well with dir="r… #15415

Suresh918 · 2019-03-07T18:46:08Z

Form field with outline is not working because Directionality service is not giving latest direction. So the direction is always 'ltr' even if we set to 'rtl'. These changes will fetch the latest direction.
Fixes #14944

…tl" angular#14944

crisbeto

I'm not sure that this is a very sustainable solution, because there's no way of notifying the consumers when the value has changed, which means that they'd have to poll for it themselves. I think that the correct solution for #14944 would be to set the direction through the dir directive, rather than setting it directly on the DOM nodes.

kyoz · 2019-03-12T14:12:34Z

Hi @crisbeto,

For this issue. I want to use dir for my whole entire app. So i'v to set and remove it manually on DOM (Base on a global variable). But it seem doesn't work well.

Is there a valid way that i can get it to work. It painful if i have to use dir for all of the component i have 😢

kyoz · 2019-03-12T14:18:07Z

I'v also tried to use dir in the wrapper container inside the app. It worked, but sometime, when i changing route, it still having this problem (Only see it on the mat-form-field with outline appearance)

I'v created an example on stackblitz

Just click 'Toggle RTL' twice and it'll happen

kyoz · 2019-03-13T03:34:27Z

I'v also tried a demo app.

To reproduce:

Go to input example page
Click LTR
Change to any other examples page and go back to input example page
Click RTL

…h dir="rtl" angular#14944" This reverts commit 501b6be

…rectionality-update

…tl" angular#14944

Suresh918 · 2019-03-15T20:29:42Z

@crisbeto
The issue is that the outline update logic (gaps, widths calculations, outline styles update etc in updateOutlineGap method). is getting executed before the element direction changes in the UI. Even if we change the direction from 'ltr' to 'rtl' using dir directive, the logic is getting executed on 'ltr' direction only. After that the direction of element is getting changed in the UI. That's why we are getting wrong outlines ('ltr' outlines to 'rtl' vice versa).

To fix this I'm storing the previous direction and doing the outline update logic on that so that UI and logic are in sync.

kyoz · 2019-03-24T14:35:44Z

@crisbeto can you review @Suresh918 's new updates. Thank you very much 👍

Suresh918 · 2019-04-07T09:56:03Z

@crisbeto @jelbourn @mmalerba Can you please review the latest changes

mmalerba · 2019-04-08T16:36:55Z

src/lib/form-field/form-field.ts

-      this._dir.change.pipe(takeUntil(this._destroyed)).subscribe(() => this.updateOutlineGap());
+      this._dir.change.pipe(takeUntil(this._destroyed)).subscribe(() => {
+        this.updateOutlineGap();
+        this._previousDirection = this._dir.value;


why is the direction not updated until after calling updateOutlineGap?

Oh sorry I just read your explanation above, can you add a comment to the code. It looks wrong at first glance, without a note explaining it.

Even looking at the description on the PR, I'm not quite sure whether this will cover all the cases. The event will only if a new value is set. Also we probably need a unit test for this.

@crisbeto @mmalerba "gulp test" and "gulp serve:devapp" are not working in my local. Do I need to do any configuration changes? Can you help me in running test cases? Even I tried "karma run", that's also giving errors in the console.

gulp test is working fine when i updated the node packages. But is there any way to run specific test suite ?

@crisbeto @mmalerba I added test cases and comments. Can you please review those.

…#15239)

This should fix the tests for AriaDescriber, AutofillMonitor, Portal, BottomSheet, Autocomplete, and Drawer.

…5255)

…15257)

…uery (angular#15267)

Reworks a test that depends on static queries and fails under Ivy.

* Fixes one unit test that was depending on static queries. * Fixes the remaining 16 failures that were due to some code that was commented out while waiting for a task which was resolved.

Fixes some tests that will fail under Ivy, because they depend on static queries.

…tl" angular#14944

…lar#15308)

…15404) This happens to work when the content is just text; however, content gets center aligned when there are matLine elements nested in a <button mat-list-item>.

googlebot · 2019-04-26T15:44:21Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

mmalerba

Looks good, thought it needs to be rebased.

CLA is fine, extra commits are shown because of a bad rebase, but all changes are actually authored by @Suresh918

mmalerba · 2019-04-26T15:41:04Z

src/lib/input/input.spec.ts

+      let wrapperElement = outlineFixture.nativeElement;
+      let outlineStart = wrapperElement.querySelector('.mat-form-field-outline-start');
+      // outlineGapPadding 5px + containerRect margin/padding in worst case 3px
+      const maxOutlineStart = '8px';


why not just make this a number?

ngbot · 2019-04-26T15:46:05Z

I see that you just added the pr: merge ready label, but the following checks are still failing:
conflicts with base branch "master"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

andrewseguin · 2019-06-26T15:48:56Z

Change passed tests internally, just needs to be rebased and we can merge it in

googlebot · 2019-06-26T15:58:53Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-06-26T15:59:20Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

mmalerba · 2019-06-26T15:59:24Z

I've gone ahead and merged in upstream/master for you @Suresh918

kyoz · 2019-09-23T06:31:59Z

@mmalerba Can we have this merge in next release, it's seem pending for a long time. Thank you ❤️

#15415) * fix(form-field): FormField with outline doesn't work well with dir="rtl" #14944 * Revert "fix(form-field): FormField with outline doesn't work well with dir="rtl" #14944" This reverts commit 501b6be * fix(form-field): FormField with outline doesn't work well with dir="rtl" #14944 * chore(ivy): add static flag to tab-related static queries * chore(ivy): add static flag to dialog-related static queries (#15239) * chore(ivy): add static flag to more queries (#15252) This should fix the tests for AriaDescriber, AutofillMonitor, Portal, BottomSheet, Autocomplete, and Drawer. * chore(ivy): add static flag to tree-related static queries (#15255) * chore(ivy): add static flag to table-related static queries (#15257) * chore(ivy): fix static query stragglers that were not marked static query (#15267) * test(menu): refactor test that depends on static queries (#15289) Reworks a test that depends on static queries and fails under Ivy. * test(sidenav): fix sidenav failures under Ivy (#15300) * Fixes one unit test that was depending on static queries. * Fixes the remaining 16 failures that were due to some code that was commented out while waiting for a task which was resolved. * test(overlay): use static queries w/ overlay containers (#15301) * test(select): update static query tests (#15320) Fixes some tests that will fail under Ivy, because they depend on static queries. * test(accordion): update static query test (#15357) * fix: mark virtual scroll viewport queries as static (#15346) * fix(form-field): FormField with outline doesn't work well with dir="rtl" #14944 * refactor(button): use [attr.disabled] host binding in MatButton (#15308) * fix(list): override native button text align in action list (#15404) This happens to work when the content is just text; however, content gets center aligned when there are matLine elements nested in a <button mat-list-item>. * fix(badge): duplicate leftover badge after server-side render (#15417) Fixes badges being left over if the current page was rendered via Angular Universal. This can cause issues, because the ids on the badges can clash. * chore: bump version to 7.3.4 w/ changelog (#15439) * fix(schematics): do not run migrations multiple times (#15424) Currently when someone runs the update schematic within a CLI project, the migration could be executed multiple times for the same `tsconfig` path This can happen because the `getProjectTsConfigPaths` function _can_ incorrectly return the same tsconfig multiple times. The paths are not properly deduped as we don't normalize the determined project tsconfig paths. e.g. `/tsconfig.json` exists and the common tsconfig path is detected. At the same time the workspace configuration specifies `tsconfig.json` in the project options. Both paths refer to the same tsconfig project, but are different strings and therefore aren't filtered out by the `Set` which contains all possible tsconfig locations --> migrations are executed for both paths. * docs(drag-drop): add docs and live example for disabled sorting (#15426) Documents the functionality from #15064 and adds a live example. * test(chip-list): rework tests to work under Ivy (#15428) Equivalent of #15427 for master. Fixes the chip list tests that are failing under Ivy due to a known breaking change. * build: throw error if example is missing a title (#15430) Throws a proper error message if one of the examples is missing a title, in order to avoid a more cryptic error down the line. * refactor(selection-list): avoid deprecated method (#15393) * use `FocusKeyManager`'s `updateActiveItem` method instead of deprecated `updateActiveItemIndex` * fix description of `MatSelectionList`'s `focus` method * remove `MatSelectionList`'s redundant `(focus)` event listener * test(table): update static query test (#15452) * refactor(stepper): rework buttons to handle Ivy (#15429) In Ivy inheriting `host` bindings works correctly by merging the two declarations, whereas in ViewEngine the bindings aren't inherited at all. This means that with our current setup the `click` listener on the buttons will be invoked twice with Ivy. These changes rework the buttons so that they work both with Ivy and ViewEngine. * fix(table): use default change detection strategy on table (#15440) This resolves an issue with Ivy where change detection was not reflecting binding changes made to the row and cell templates in the component declaring the table. In View Engine, change detection for a dynamically created view runs in two cases: 1) When the view in which it was inserted is checked 2) When the view in which it was declared is checked As a result, if a dynamic view is inserted into an OnPush component, that view is not actually checked only "on push". It is also checked as often as its declaration view is checked (even if the insertion view is explicitly detached from change detection). For this reason, marking `MatTable` as "OnPush" doesn't have much of an effect. It is built almost entirely of views that are declared elsewhere, so its change detection frequency is dependent on those declaration views. It also doesn't have any bindings inside its own view that would be protected by "OnPush", so marking it this way is effectively a noop and can be removed without performance regressions. In Ivy, this change detection loophole has been closed, so declaration views can no longer de-optimize OnPush components. This means bindings inherited from declaration views aren't updated by default if `MatTable` is OnPush (the embedded view would need to be marked dirty explicitly). To avoid breaking changes when we transition to Ivy, it makes more sense to perpetuate the current behavior by removing the "OnPush" marker. As delineated earlier, this has no impact on View Engine. For Ivy, it would ensure bindings inherited from the declaration view are still checked by default. * test(input): make the type validation test to pass in Ivy (#15460) * fix(stepper): avoid breaking change in stepControl type (#15464) In #15134 we reworked the stepper not to depend on `@angular/forms` under the assumption that our limited `FormControl` interface would be enough to avoid a breaking change. Some people ended up being broken by the change so this PR reworks the `stepControl` type to avoid the breaking change. Fixes #15462. * fix(tooltip): render style values in ngOnInit instead of the constructor (#15469) Due to changes in Angular it's not logical to change style values directly within a constructor. This should be done instead within the ngOnInit lifecycle hook. This patch changes this behavior for the `ToolTip` component. * chore: update tooltip golden file (#15479) Fixes the API golden check failing against master. * refactor(datepicker): adapt custom header demo for Ivy (#15458) * refactor(focus-monitor): adapt demo examples for Ivy (#15475) * fix(table): use static queries for examples (#15483) * fix(stepper): content not being rendered out initially with ivy (#15485) Fixes the stepper's content not being shown on the first render with Ivy, because we assume that the template with be present on init. * Revert "fix(form-field): FormField with outline doesn't work well with dir="rtl" #14944" This reverts commit 501b6be * fix(form-field): FormField with outline doesn't work well with dir="rtl" #14944 * fix(form-field): test cases added for form field outline styles w.r.t direction changes * fix(form-field): lint errors and test cases fixes * fix(form-field): lint errors and test cases fixes * fix(form-field): test cases changes and comments added * fix(form-field): lint error fix (cherry picked from commit 914d40e)

These changes undo the changes from angular#15415 because they were making incorrect assumptions about the page starting off as LTR, and they were calling `updateOutlineGap` with an outdated value for the direction. This is causing issues like angular#17390. I'm not totally sure what was being fixed by these changes, because the test that was added didn't fail even if I reverted all of the changes. From what I can tell the problem angular#15415 was trying to solve was that the outline gap might be updated too early on a direction change, before the browser has had the chance to recalculate the layout. These changes switch to recalculating inside a `requestAnimationFrame` after direction changes, in order to give the browser enough time to recalculate the layout. Fixes angular#17390.

These changes undo the changes from #15415 because they were making incorrect assumptions about the page starting off as LTR, and they were calling `updateOutlineGap` with an outdated value for the direction. This is causing issues like #17390. I'm not totally sure what was being fixed by these changes, because the test that was added didn't fail even if I reverted all of the changes. From what I can tell the problem #15415 was trying to solve was that the outline gap might be updated too early on a direction change, before the browser has had the chance to recalculate the layout. These changes switch to recalculating inside a `requestAnimationFrame` after direction changes, in order to give the browser enough time to recalculate the layout. Fixes #17390.

angular-automatic-lock-bot · 2019-10-28T00:03:29Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

fix(form-field): FormField with outline doesn't work well with dir="r…

501b6be

…tl" angular#14944

Suresh918 requested review from jelbourn and mmalerba as code owners March 7, 2019 18:46

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 7, 2019

crisbeto reviewed Mar 7, 2019

View reviewed changes

Suresh918 added 3 commits March 16, 2019 01:24

Revert "fix(form-field): FormField with outline doesn't work well wit…

2dd29e0

…h dir="rtl" angular#14944" This reverts commit 501b6be

Merge branch 'master' of https://github.com/angular/material2 into di…

0013ac5

…rectionality-update

fix(form-field): FormField with outline doesn't work well with dir="r…

1ca5bce

…tl" angular#14944

mmalerba reviewed Apr 8, 2019

View reviewed changes

kara and others added 15 commits April 15, 2019 19:27

chore(ivy): add static flag to tab-related static queries

63ca947

chore(ivy): add static flag to dialog-related static queries (angular…

ba98344

…#15239)

chore(ivy): add static flag to more queries (angular#15252)

d06b0bf

This should fix the tests for AriaDescriber, AutofillMonitor, Portal, BottomSheet, Autocomplete, and Drawer.

chore(ivy): add static flag to tree-related static queries (angular#1…

cbd527c

…5255)

chore(ivy): add static flag to table-related static queries (angular#…

a22cbbb

…15257)

chore(ivy): fix static query stragglers that were not marked static q…

2df2af2

…uery (angular#15267)

test(menu): refactor test that depends on static queries (angular#15289)

050af72

Reworks a test that depends on static queries and fails under Ivy.

test(sidenav): fix sidenav failures under Ivy (angular#15300)

4e61755

* Fixes one unit test that was depending on static queries. * Fixes the remaining 16 failures that were due to some code that was commented out while waiting for a task which was resolved.

test(overlay): use static queries w/ overlay containers (angular#15301)

6adbdb2

test(select): update static query tests (angular#15320)

77c13c8

Fixes some tests that will fail under Ivy, because they depend on static queries.

test(accordion): update static query test (angular#15357)

864b238

fix: mark virtual scroll viewport queries as static (angular#15346)

4f08d26

fix(form-field): FormField with outline doesn't work well with dir="r…

6743041

…tl" angular#14944

refactor(button): use [attr.disabled] host binding in MatButton (angu…

4338173

…lar#15308)

fix(list): override native button text align in action list (angular#…

705211b

…15404) This happens to work when the content is just text; however, content gets center aligned when there are matLine elements nested in a <button mat-list-item>.

mmalerba added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 26, 2019

mmalerba reviewed Apr 26, 2019

View reviewed changes

mmalerba approved these changes Apr 26, 2019

View reviewed changes

mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Apr 26, 2019

mmalerba mentioned this pull request May 13, 2019

The label on a mat form field with Outline with a prefix not aligning inside a dialog #15027

Closed

andrewseguin added the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label May 30, 2019

andrewseguin added the pr: needs rebase label Jun 26, 2019

Merge remote-tracking branch 'upstream/master' into pr15415

08a7c28

googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Jun 26, 2019

mmalerba added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Jun 26, 2019

andrewseguin removed the pr: needs rebase label Jun 26, 2019

andrewseguin merged commit 914d40e into angular:master Sep 27, 2019

crisbeto mentioned this pull request Oct 14, 2019

fix(form-field): incorrect assumptions about page direction #17395

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 28, 2019

fix(form-field): FormField with outline doesn't work well with dir="r… #15415

fix(form-field): FormField with outline doesn't work well with dir="r… #15415

Uh oh!

Conversation

Suresh918 commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

kyoz commented Mar 12, 2019

Uh oh!

kyoz commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyoz commented Mar 13, 2019

Uh oh!

Suresh918 commented Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyoz commented Mar 24, 2019

Uh oh!

Suresh918 commented Apr 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmalerba Apr 8, 2019

Choose a reason for hiding this comment

Uh oh!

mmalerba Apr 8, 2019

Choose a reason for hiding this comment

Uh oh!

crisbeto Apr 8, 2019

Choose a reason for hiding this comment

Uh oh!

Suresh918 Apr 15, 2019

Choose a reason for hiding this comment

Uh oh!

Suresh918 Apr 15, 2019

Choose a reason for hiding this comment

Uh oh!

Suresh918 Apr 15, 2019

Choose a reason for hiding this comment

Uh oh!

googlebot commented Apr 26, 2019

Uh oh!

mmalerba left a comment

Choose a reason for hiding this comment

Uh oh!

mmalerba Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

ngbot bot commented Apr 26, 2019

Uh oh!

andrewseguin commented Jun 26, 2019

Uh oh!

googlebot commented Jun 26, 2019

Uh oh!

googlebot commented Jun 26, 2019

Uh oh!

mmalerba commented Jun 26, 2019

Uh oh!

kyoz commented Sep 23, 2019

Uh oh!

angular-automatic-lock-bot bot commented Oct 28, 2019

Uh oh!

Uh oh!

Suresh918 commented Mar 7, 2019 •

edited

Loading

kyoz commented Mar 12, 2019 •

edited

Loading

Suresh918 commented Mar 15, 2019 •

edited

Loading

Suresh918 commented Apr 7, 2019 •

edited

Loading