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

FW-1141 (requiredParents insertion mechanism) #29219

Conversation

Projects
None yet
3 participants
@pkozlowski-opensource
Copy link
Member

commented Mar 11, 2019

This PR removes the requiredParents insertion mechanism.

@googlebot googlebot added the cla: yes label Mar 11, 2019

@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:fw_1141_reproduce_test branch from f3669c9 to 50ec7bf Mar 11, 2019

@kara

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:fw_1141_reproduce_test branch from 50ec7bf to e297788 Mar 11, 2019

@ngbot ngbot bot added this to the needsTriage milestone Mar 11, 2019

@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review Mar 11, 2019

@pkozlowski-opensource pkozlowski-opensource requested review from angular/fw-compiler as code owners Mar 11, 2019

@pkozlowski-opensource pkozlowski-opensource changed the title Fw 1141 reproduce test FW-1141 (requiredParents insertion mechanism) Mar 12, 2019

@kara

kara approved these changes Mar 13, 2019

Copy link
Contributor

left a comment

LGTM

@ngbot

This comment has been minimized.

Copy link

commented Mar 13, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure conflicts with base branch "master"
    pending status "google3" is pending

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.

@kara

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

@pkozlowski-opensource FYI i'm going to run a "global" presubmit tonight to make sure screenshot tests are also healthy (normally we just run against our a selection of projects). If it passes, I'll merge it first thing in the morning.

@kara

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Note: presubmit passes with a patch CL. Need to rebase this and then we should be able to get it in.

fix(core): don't wrap `<tr>` and `<col>` elements into a required parent
BREAKING CHANGE:

Certain elements (like `<tr>` or `<col>`) require parent elements to be of a certain type by the HTML specification
(ex. <tr> can only be inside <tbody> / <thead>). Before this change Angular template parser was auto-correcting
"invalid" HTML using the following rules:
- `<tr>` would be wrapped in `<tbody>` if not inside `<tbody>`, `<tfoot>` or `<thead>`;
- `<col>` would be wrapped in `<colgroup>` if not inside `<colgroup>`.

This meachanism of automatic wrapping / auto-correcting was problematic for several reasons:
- it is non-obvious and arbitrary (ex. there are more HTML elements that has rules for parent type);
- it is incorrect for cases where `<tr>` / `<col>` are at the root of a component's content, ex.:

```html
<projecting-tr-inside-tbody>
  <tr>...</tr>
</projecting-tr-inside-tbody>
```

In the above example the `<projecting-tr-inside-tbody>` component culd be "surprised" to see additional
`<tbody>` elements inserted by Angular HTML parser.

@kara kara force-pushed the pkozlowski-opensource:fw_1141_reproduce_test branch 2 times, most recently from d463d10 to b235cc8 Mar 13, 2019

@kara kara force-pushed the pkozlowski-opensource:fw_1141_reproduce_test branch from b235cc8 to d6bb919 Mar 13, 2019

@kara

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

merge-assistance: global approval

@matsko matsko closed this in f2dc32e Mar 14, 2019

matsko added a commit that referenced this pull request Mar 14, 2019

wKoza added a commit to wKoza/angular that referenced this pull request Apr 17, 2019

fix(core): don't wrap `<tr>` and `<col>` elements into a required par…
…ent (angular#29219)

BREAKING CHANGE:

Certain elements (like `<tr>` or `<col>`) require parent elements to be of a certain type by the HTML specification
(ex. <tr> can only be inside <tbody> / <thead>). Before this change Angular template parser was auto-correcting
"invalid" HTML using the following rules:
- `<tr>` would be wrapped in `<tbody>` if not inside `<tbody>`, `<tfoot>` or `<thead>`;
- `<col>` would be wrapped in `<colgroup>` if not inside `<colgroup>`.

This meachanism of automatic wrapping / auto-correcting was problematic for several reasons:
- it is non-obvious and arbitrary (ex. there are more HTML elements that has rules for parent type);
- it is incorrect for cases where `<tr>` / `<col>` are at the root of a component's content, ex.:

```html
<projecting-tr-inside-tbody>
  <tr>...</tr>
</projecting-tr-inside-tbody>
```

In the above example the `<projecting-tr-inside-tbody>` component culd be "surprised" to see additional
`<tbody>` elements inserted by Angular HTML parser.

PR Close angular#29219

wKoza added a commit to wKoza/angular that referenced this pull request Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.